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

Improve kotlin.uuid.Uuid hashcode #5366

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FelixDes
Copy link

@FelixDes FelixDes commented Oct 3, 2024

Hi, dear maintainers,

I've found an issue with the new multiplatform implementation of kotlin.uuid.Uuid.
Its hashCode method can be rewritten with utilizing Long.hashCode() for better readability.

@FelixDes
Copy link
Author

FelixDes commented Oct 3, 2024

The math underlying this change:

x = mostSignificantBits ^ leastSignificantBits
Original code was:

int( x ) ^ int( x >> 32 )

With my change it is:

int( x ^ (x >>> 32) ) <=> int( x ) ^ int( x >>> 32 )

>>> and >> are equal in this case because the difference is in the left most bit which is truncated by int casting

@fzhinkin
Copy link
Contributor

fzhinkin commented Oct 7, 2024

cc @qurbonzoda

@qurbonzoda
Copy link
Contributor

The two implementations are indeed equivalent according to Long.hashCode() documentation: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Long.html#hashCode()
I am not sure about the improved readability, it depends on whether the reader is familiar with how Long.hashCode() is implemented.

@qurbonzoda qurbonzoda self-requested a review October 9, 2024 13:28
@qwwdfsad
Copy link
Contributor

qwwdfsad commented Oct 9, 2024

The readability is straightforward -- instead "hey, is it something tailored by UUID? Is it handrolled? Is it robust for hashmaps?" there is immediate "Ok, this is how the language hashes longs"

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.

4 participants