-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
gh-123497: New limit for Python integers on 64-bit platforms #123498
base: main
Are you sure you want to change the base?
gh-123497: New limit for Python integers on 64-bit platforms #123498
Conversation
Instead of be limited just by the size of addressable memory (2**63 bytes), Python integers are now also limited by the number of bits, so the number of bit now always fit in 64-bit integer. Both limits are much larger than what can be available on practice, so there is no effect on users. _PyLong_NumBits() and _PyLong_Frexp() are now always successful.
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.
Overall, the change LGTM, but it would be easier to review and it may make more sense if splitted in 3 parts:
- change MAX_LONG_DIGITS limit and related changes -- maybe put long_rshift() and long_lshift() in this part
- change _PyLong_Frexp()
- _PyLong_NumBits() is always successful, bit_length(), bit_count(), etc.
Is it necessary? The PR is not so large and I can split it on even smaller part parts, but this is an additional work and is prone for errors. I can not guarantee that the intermediate state will be correct. |
@@ -3505,21 +3494,13 @@ _PyLong_Frexp(PyLongObject *a, int64_t *e) | |||
/* Rescale; make correction if result is 1.0. */ | |||
dx /= 4.0 * EXP2_DBL_MANT_DIG; | |||
if (dx == 1.0) { | |||
if (a_bits == INT64_MAX) | |||
goto overflow; | |||
assert(a_bits < UINT64_MAX); |
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.
uint64_t a_bits;
cannot be greater than UINT64_MAX.
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.
Yes, but it can be equal.
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.
LGTM. See also @erlend-aasland's coding style suggestions.
Co-authored-by: Victor Stinner <vstinner@python.org>
I have no idea what are the problems with building on Windows. Building locally fails with the same error, but running the reported command manually succeeded. And it seems that |
Fixed building on Windows. MAX_LONG_DIGITS was incorrectly set on 32-bit platforms. #123724 is an alternative PR that uses capi-workgroup/api-evolution#52 is a discussion about using unsigned or signed types. I have no strong preferences, although the code with unsigned types is slightly simpler (bit count is non-negative by its nature). |
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.
LGTM
In terms of the implementation, I prefer unsigned, it avoids to check if a number is negative by design (even for assertions). So, what do you choose? |
Instead of be limited just by the size of addressable memory (2**63 bytes), Python integers are now also limited by the number of bits, so the number of bit now always fits in 64-bit integer.
Both limits are much larger than what can be available on practice, so there is no effect on users.
_PyLong_NumBits() and _PyLong_Frexp() are now always successful.