GeezerVison is showing github.com — bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong by mdickinson · Pull Request #30496 · python/cpython
Conversation
mdickinson
commented
Jan 9, 2022
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
PR #27832 inadvertently introduced a couple of changes to PyLong_FromLong that didn't make a lot of sense: an (unsigned long) cast was replaced with (twodigits), and a digit count variable (counting number of PyLong digits in a C long) had its type needlessly changed from int to Py_ssize_t.
- The first change is a potential portability bug, but only on platforms with 128-bit longs. The
(unsigned long) cast is obviously correct, while figuring out whether (twodigits) loses information takes some work.
- The second change is merely a potential pessimization: there's no need to use what's typically a 64-bit integer to count the number of PyLong digits in a
long.
This PR:
- reverts those two changes
- moves the check for medium values earlier in the function (immediately after the small values check), and simplifies that check
- makes the code a bit less branchy (from experiments locally and on godbolt.org, the expression
ival < 0 ? 0U-(unsigned long)ival : ival) gets compiled to something branchless on most platforms, as does ival < 0 ? -ndigits : ndigits
- introduces parallel changes for
PyLong_FromLongLong, which now has a fast path for medium-size values.
https://bugs.python.org/issue46311
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I don't see how this qualifies for skip issue. Perhaps I am misreading the devguide, but to me this PR seems inappropriately labelled. If I am mistaken; sorry for the noise.
|
mdickinson reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
mdickinson
changed the title
Clean up PyLong_FromLong and PyLong_FromLongLong
bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong
Jan 9, 2022
|
|
}
|
|
|
|
|
|
/* Create a new int object from a C long int */
|
|
|
|
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.
This is a minor consistency fix, following the Boy Scout Rule: the prevailing style in this file is to leave a blank line between the comment-description and the definition. But the change is not strictly necessary for this PR, and I can revert if reviewers prefer.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks. Looks good on initial review.
There are a couple of warnings (lines 294 and 1118) that need fixing.
I re-review once those are fixed.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Two changes that should silence compiler warnings.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks, @markshannon. The compiler warnings have been silenced.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|