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

Fix hash computation for JRuby's RubyMessage #2144

Merged
merged 1 commit into from
Sep 20, 2016

Conversation

abscondment
Copy link
Contributor

@abscondment abscondment commented Sep 20, 2016

#2037 added a test that exposed a bug in the JRuby extension: System.identityHashCode returns a hash that does not consider a Message's values. This means two Messages with identical values will not have identical hashCodes.

This patch uses the pattern from RubyMap to combine the hashCodes from all values in a given message and produce a unique, consistent, value-based hash.

`System.identityHashCode` returns a hash that does not consider a
Message's values. This means two Messages with identical values will not
have identical hashCodes.

This patch uses the pattern from RubyMap to combine the hashCodes from
all values in a given message and produce a unique, consistent,
value-based hash.
@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@abscondment
Copy link
Contributor Author

@haberman Here's the fix for the JRuby breakage - thanks!

@haberman
Copy link
Member

Thanks! I will merge it once the tests pass.

@abscondment
Copy link
Contributor Author

AppVeyor seems to be tripping over itself.

Build execution time has reached the maximum allowed time for your plan (60 minutes).

@haberman
Copy link
Member

Travis is good enough for me here. Thanks again for the fix!

@haberman haberman merged commit 4f379f8 into protocolbuffers:master Sep 20, 2016
@abscondment abscondment deleted the fix-jruby-hash branch September 20, 2016 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants