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

32-bit support for xxHash algorithm #2

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 27, 2020

Closes #1

@ghost ghost changed the title #1 32-bit support for xxHash algorithm 32-bit support for xxHash algorithm Jan 27, 2020
@ghost ghost mentioned this pull request Jan 27, 2020
@ekpyron
Copy link
Owner

ekpyron commented Jan 27, 2020

Can you perhaps add a handful of canonical test cases to test.cpp? Then I'd be happy to merge this, thank you very much!

xxh32.hpp Outdated

struct xxh32 {
static constexpr uint32_t hash( const char *p, uint32_t len, uint32_t seed ) {
return finalize( (len >= 16 ? h32bytes(p, len, seed) : seed+P5) + len, p+(len & ~0xF), len & 0xF );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return finalize( (len >= 16 ? h32bytes(p, len, seed) : seed+P5) + len, p+(len & ~0xF), len & 0xF );
return finalize( (len >= 16 ? h16bytes(p, len, seed) : seed+P5) + len, p+(len & ~0xF), len & 0xF );

@ekpyron
Copy link
Owner

ekpyron commented Jan 27, 2020

As a matter of fact, I just tried to extend the tests in test.cpp to your 32-bit implementation and checked against the reference implementation https://github.com/Cyan4973/xxHash/, but not all tests succeed, so there must be some small incorrectness in the implementation - I'll have a more detailed look later.

@ghost
Copy link
Author

ghost commented Feb 3, 2020

To be honest, it's been a long time since I wrote this and as far as I can remember, I didn't came into any problems when I used it. But anyway, I'll look at it soon and make any edits needed.

@ekpyron
Copy link
Owner

ekpyron commented Feb 3, 2021

I'm just noticing that shockingly this has been open for a year now... I'll try looking into it soon, but I can't promise when I'll get around to.

@ghost ghost marked this pull request as draft February 4, 2021 15:50
@ghost ghost force-pushed the master branch 2 times, most recently from 6aabeb5 to eaa58e4 Compare February 4, 2021 21:15
@ghost ghost marked this pull request as ready for review February 4, 2021 21:24
@ghost
Copy link
Author

ghost commented Feb 4, 2021

Sorry for the force-push mess above, I initially committed with an old account name and wanted to get rid of it.

I forgot about this pull request but finally took the necessary time to finish it.
I added the tests to test.cpp as you requested and corrected my implementation to make them pass.

@ghost ghost requested a review from ekpyron February 4, 2021 21:28
Copy link
Owner

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you very much!

@ekpyron ekpyron merged commit 72e55c8 into ekpyron:master Feb 5, 2021
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.

Support for 32-bit xxHash algorithm
1 participant