Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUE-393: Fix incorrect equality semantics when unifying similar obj… #397

Merged
merged 2 commits into from
Jun 25, 2018

Conversation

rbrush
Copy link
Contributor

@rbrush rbrush commented Jun 22, 2018

Fix for #393 by simply wrapping potential keys in a Java object that explicitly uses Clojure's equality and hash logic.


Object
(equals [this other]
(= wrapped (.wrapped ^JavaEqualityWrapper other)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are assuming the other is always going to be a JavaEqualityWrapper, otherwise there could be cast exceptions. I believe that is fine though, since this is only intended to be used/compared in collections that contain only these JavaEqualityWrapper wrapper types.

To confirm: this is your reasoning here, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the only use case I had in mind, but it's a good idea to add an instance? check here. They're quite cheap and makes this class more general.

Copy link
Collaborator

@mrrodriguez mrrodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@rbrush rbrush merged commit ce65c50 into master Jun 25, 2018
@rbrush rbrush deleted the issue-393 branch June 25, 2018 13:30
@rbrush
Copy link
Contributor Author

rbrush commented Jun 25, 2018

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants