-
Notifications
You must be signed in to change notification settings - Fork 122
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
Fix Quantity hashcode method. #307
Conversation
Would you mind adding tests. This is serious enough to warrant some automated test |
I've added simple hashcode test. Is this enough? :) @cquiroz |
Anyone? @garyKeorkunian @cquiroz @nornagon |
That looks good for me |
Ok, so could you ack the review, because I cannot merge it without your permission? And what about releasing the new version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the first test, it just looks like an identity test, but I don't object.
Thanks for the PR. Sorry for the delay. |
Whenever hashcode is invoked on the same object it should return the same value. This is the reason for this test. Thanks! :) |
@@ -176,7 +178,9 @@ abstract class Quantity[A <: Quantity[A]] extends Serializable with Ordered[A] { | |||
* | |||
* @return | |||
*/ | |||
override def hashCode() = toString.hashCode() | |||
override def hashCode() = { | |||
Objects.hash(dimension, Double.box(to(dimension.primaryUnit))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the resulting hashCode is not stable across JVM runs because the dimension
itself may not have a stable hashCode
.
Hi,
I've fixed hashcode method in Quantity class. The previous one was breaking the equals/hashcode conduct. It is a very serious bug, so please review it and merge if only possible.