-
Notifications
You must be signed in to change notification settings - Fork 37
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
Result is a Signed 32bit value, is this known and wanted? #4
Comments
This is indeed known, the behavior is documented in the README:
Is it desired? There were a few reasons for settling on the signed version:
If you run this in node with the flags
|
@SheetJSDev Thanks for clarifying, especially regarding the performance penalty. I didn't think of that. will try to move my code to do the unsigned conversion at the very end of the checksum calculation. the python example is interesting, as this seems to use the same binding as my ruby example. You never cease to learn :) I'm currently using a somewhat patched version of your lib in my code anyway as I needed continuous summing, so I'll close this issue. |
I just spent a good hour trying to figure out why the results of the JS implementation did not match other implementation (I've specifically tested it against ruby's zLib wrapper) and then I understood: All the bitwise operators in JavaScript treat the variables as 32bit signed values. This is not a problem per se, as the bitwise operations are usually not dependent on the signs, but unfortunately the output is also 32bit signed. I've looked at various implementations now and it seems to me that usually, the output value of the crc process should be an unsigned 32 bit value.
Here's a simple test case (the numbers are not very relevant, it just takes a certain combination to trigger the sign problem):
If you convert this value to an unsigned value (Stack Overflow never disappoints) by
CRC.buf(a) >>> 0
, you get the correct result.I've also checked this against the crc32 utility of Mac OS X by converting the numbers into a text file and the result matches.
I''ll do a pull request if this makes things easier.
The text was updated successfully, but these errors were encountered: