-
Notifications
You must be signed in to change notification settings - Fork 187
use zlib-rs for crc32 (when available)
#526
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
Conversation
41e7174 to
c584e97
Compare
|
Unfortunately this library exposing a There is probably some way around that in zlib-rs, but I think it would impact our performance for the cases that we care about. Anyhow, what I've implemented now is conceptually no worse than what crc-fast and crc32fast do, so I think it should be fine. |
|
With that this should be OK to merge. We might change the path of where the crc are exported at some point in the future, but that should be straightforward to handle when bumping the dependency version in flate2. |
|
It seems you have been nerd-sniped here, and… I know that feeling :). But let me start with: This is awesome, and thanks so much, I can't wait for this to be merged. However, I haven't looked at the PR yet because I think it could be rebased onto #527 once that is merged. Could you take a look to see if that would truly be reducing risk? Or maybe #527 could be improved even. Oh, and if you want, would it be possible to add a |
5decf61 to
aef26ac
Compare
I had thought about it before, but yes.
Those tests are useful just to generally make sure the crc looks good. In this PR I'm also testing the public API (both with crc32fast and without), so that even the parts that
Yes, I added one. |
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, I think that's it!
This currently hits a bug in the zlib-rs implementation that we don't run into internally, but that the public API does. So, I'll need to go fix that. But hopefully this gives an impression what it'll look like.