FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong by mdickinson · Pull Request #30496 · python/cpython · GitHub

/ cpython Public

bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong - #30496

Merged
markshannon merged 3 commits into
python:mainfrom
mdickinson:cleanup-py-long-from-long
Mar 1, 2022
Merged

bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong#30496
markshannon merged 3 commits into
python:mainfrom
mdickinson:cleanup-py-long-from-long

Conversation

mdickinson commented Jan 9, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member Author

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

mdickinson changed the title Clean up PyLong_FromLong and PyLong_FromLongLong bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong Jan 9, 2022
Comment thread Objects/longobject.c
}

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
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 self-requested a review January 21, 2022 15:01

mdickinson left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Two changes that should silence compiler warnings.

Comment thread Objects/longobject.c Outdated
Comment thread Objects/longobject.c Outdated

Copy link
Copy Markdown
Member Author

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

markshannon self-assigned this Feb 28, 2022

Copy link
Copy Markdown
Member

Thanks

markshannon merged commit c60e6b6 into python:main Mar 1, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL