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 BigInt#to_*64* and #hash #10121

Merged
merged 3 commits into from
Jan 6, 2021
Merged

Fix BigInt#to_*64* and #hash #10121

merged 3 commits into from
Jan 6, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Dec 20, 2020

Fixes #10107
Closes #10115


The main cause of the old problems is that LibGMP.get_ui and get_si return basically garbage if the number would have overflowed. But the code assumed that it truncates the number correctly, same as Crystal.

https://gmplib.org/manual/Converting-Integers

  • mpz_get_ui

    The sign of op is ignored, only the absolute value is used.

  • mpz_get_si

    If op is too big to fit in a signed long int, the returned result is probably not very useful.

So this takes matters into our own hands and pre-truncates the number with modulo where appropriate.


Another issue was that for non-! variants it was checking only for negative arguments, but all other cases silently "garbage-overflowed".


This only fixes the *-64 variants, the others are still broken, though hopefully not as horribly.


The hash itself obviously can just work with to_i64! and actually there's no better implementation than that, so I just fixed it.

I'm actually having some difficulty in making the hashes equal with both Int64 < 0 and UInt64 > Int64::MAX at the same time, so I chose to skip the latter (so, for numbers greater than Int64::MAX hashes across UInt64 and BigInt don't actually match). Too difficult to fix everything for now.

It's curious, it seems like the code at the bottom of the file big_int.cr tries to fix this, but the code is unused and became broken!

else
to_s.to_i64
end
(self % BITS64).to_u64.to_i64!
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse to_u64!?

Suggested change
(self % BITS64).to_u64.to_i64!
to_u64!.to_i64!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see what you mean. Hmm actually no, I don't like it. The code happens to match, but I think this better shows the intent that this roundtrip is intentional.

@straight-shoota straight-shoota added this to the 1.0.0 milestone Dec 30, 2020
@asterite asterite merged commit c6669fe into crystal-lang:master Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for clarification regarding BigInt hashing
4 participants