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

Follow MRI implementation for RubyArray hash #2443

Merged
merged 1 commit into from Jan 12, 2015
Merged

Conversation

Who828
Copy link
Contributor

@Who828 Who828 commented Jan 9, 2015

This fixes #2437 and follows MRI approach for RubyArray hash method, however for consistency across the codebase I have made use of SipHashInline and PerlHash unlike MRI which makes use of MurMurHash.

@headius
Copy link
Member

@headius headius commented Jan 12, 2015

Seems good to use the same hashing algorithm as for Strings. Let's go with it.

headius added a commit that referenced this issue Jan 12, 2015
Follow MRI implementation for RubyArray hash
@headius headius merged commit 4ed53f8 into jruby:master Jan 12, 2015
1 check failed
headius added a commit that referenced this issue Jan 12, 2015
This reverts commit 4ed53f8, reversing
changes made to 3a19c89.
@headius
Copy link
Member

@headius headius commented Jan 12, 2015

Bleh, sorry, I botched this review. I had to revert.

Don't use toString to get bytes for the string. I'm almost positive MRI does not convert objects to String before including them in hash calculation. I believe it just uses their natural hash() result, which may be why they don't use murmurhash (murmur is for hashing bytes).

Wanna take another shot at it? Use the MRI code for reference, and if it doesn't make sense to use murmur/sip/perlhash because we don't have bytes[], then just do what MRI does.

@headius headius added this to the Invalid or Duplicate milestone May 4, 2015
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