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

Allow to parameterize seed value as in original murmur3 implementation #15

Merged
merged 1 commit into from
Aug 19, 2017

Conversation

mbyczkowski
Copy link
Contributor

When I was implementing this change, I missed that similar work has been done already (#9 and #10), but I figured it's worth sharing.

  • added seed to every constructor (32-, 64- and 128-bit) as well as Sum32, Sum64 and Sum128
  • made sure that non-zero seeds are also tested for correctness (used other ref implementations to make sure that seeded outputs are correct and added them in test file)
  • my fork has been running in production already 🙂

Obvious drawback is the API breakage of this lib, so maybe the way to go would be to tag the lib and start versioning (e.g. 0.1 for current, and 0.2 for this PR and up)?

@spaolacci
Copy link
Owner

Thanks for the PR. I had roughly equivalent stahed patch that I uploaded this morning in 4ec5a0f. The API is extended (not altered) with seed aware constructors and top-level accessors.

It obiously conflicted your patch set, but I'd like to have at through since there's a few well deserved nitpicking, and I also find your seed slot location more ingenious than mine.

I can cherry pick those extra changes manually, it's up to you.

Thanks.

@mbyczkowski mbyczkowski force-pushed the master branch 5 times, most recently from 809aab7 to bbdbfec Compare August 7, 2017 22:24
@mbyczkowski
Copy link
Contributor Author

Extending the API was probably the right move. I have rebased my PR based on your changes and preserved the stuff the I believe you wanted to keep. Lemme know what you think!

@spaolacci spaolacci merged commit dbc0f40 into spaolacci:master Aug 19, 2017
@spaolacci
Copy link
Owner

All merged. Thanks!

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