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

Added javadoc messages concerning hash64 seed and sign extension. #34

Merged
merged 1 commit into from Dec 28, 2019

Conversation

Claudenw
Copy link
Contributor

Fix for CODEC-275

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.321% when pulling ee89227 on Claudenw:CODEC-275_Update_javadoc into e4de423 on apache:master.

@aherbert
Copy link
Contributor

I am 50/50 on this.

I would not call it a bug when there is no reference hash64 implementation. This method is a port of a bad adaption of the hash128x86 method to output a 64-bit hash. So unlike the other method where it was a bug because the output was not the same as the reference implementation, this can be labelled as an implementation detail.

The only real effect is that the initial hash will be xor'd with 1's instead of 0's for the upper 32-bits and if the seed is negative.

Since we do not know about the statistical properties of this hash, it is not part of the original code and it is marked as deprecated then adding a lot of javadoc about sign-extension bugs which do not manifest (because the default seed is positive) to 5 deprecated methods seems like noise to me.

I would update the javadoc on the main method which accepts a seed to state that the seed is not treated as unsigned.

@garydgregory
Copy link
Member

I am going to bring this in because it helps understand the code more, especially since these comments are all on deprecated methods.

@garydgregory garydgregory merged commit e918527 into apache:master Dec 28, 2019
@garydgregory
Copy link
Member

asfgit pushed a commit that referenced this pull request Dec 28, 2019
@Claudenw Claudenw deleted the CODEC-275_Update_javadoc branch December 28, 2019 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants