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

Fix crc32 overload #39

Closed
wants to merge 5 commits into from

Conversation

n-nishizawa
Copy link

In order to fix crc32 overload issue, I introduced the built-in crc32 Zlib.crc32.

This version is no longer compatible with previous versions.

I'm not used to ruby native extension coding, so please review it and give me some advice.

@igrigorik
Copy link
Owner

Hmm, not sure this is the right approach.. If I'm reading this correctly, you removed crc32 step and replaced it with a simple modulo? The pure Ruby version looks good, but the native implementation needs to call the Ruby's crc32.

@n-nishizawa
Copy link
Author

Thank you so much for your review and advice.

The index is computed based on Zlib.crc32.
https://github.com/igrigorik/bloomfilter-rb/pull/39/files#diff-127d145f47af4cdb51891aad435aba5aR28

And the Zlib.crc32 is given to native code through block.
(So index is not simple modulo.)
https://github.com/igrigorik/bloomfilter-rb/pull/39/files#diff-5fcfefdfe790e195f24687d8aac68f5dR90

I'm not sure how to call ruby's built-in crc32 directly from native code, so I used block mechanism to pass the hash function to native code.

@igrigorik
Copy link
Owner

@n-nishizawa ah, gotcha.. missed the block callback.

That's going to be a costly roundtrip. Let's see if we can make it a direct call..

@n-nishizawa
Copy link
Author

Thank you very much for your comment.
I'm worried about the cost issue too.

Yeah, I also will find how to call the crc32 directly.

@igrigorik igrigorik closed this Dec 3, 2022
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.

crc32 is overridden by another library when using bundler
2 participants