Skip to content

bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong #30496

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

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Jan 9, 2022

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

@erlend-aasland
Copy link
Contributor

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
Copy link
Member Author

@erlend-aasland Yes, I think you're right. I'll add an issue.

@mdickinson mdickinson changed the title Clean up PyLong_FromLong and PyLong_FromLongLong bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong Jan 9, 2022
@@ -274,44 +274,40 @@ _PyLong_Negate(PyLongObject **x_p)
}

/* Create a new int object from a C long int */

Copy link
Member Author

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.

@markshannon
Copy link
Member

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.

@markshannon markshannon self-requested a review January 21, 2022 15:01
Copy link
Member Author

@mdickinson mdickinson left a 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.

@mdickinson
Copy link
Member Author

Thanks, @markshannon. The compiler warnings have been silenced.

@markshannon markshannon self-assigned this Feb 28, 2022
@markshannon
Copy link
Member

Thanks

@markshannon markshannon merged commit c60e6b6 into python:main Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants