GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong by mdickinson · Pull Request #30496 · python/cpython

Conversation

@mdickinson

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mdickinson
Clean up PyLong_FromLong and PyLong_FromLongLong
5cf8d94
@mdickinson mdickinson added skip issue skip news labels Jan 9, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 9, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Jan 9, 2022
@mdickinson mdickinson mentioned this pull request Jan 9, 2022
bpo-44946: Streamline operators and creation of ints for common case of single 'digit'. #27832
Merged
@erlend-aasland

erlend-aasland commented Jan 9, 2022

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.

mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson

mdickinson commented Jan 9, 2022

Copy link
Copy Markdown
Member Author

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

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson mdickinson changed the title Clean up PyLong_FromLong and PyLong_FromLongLong bpo-46311: Clean up PyLong_FromLong and PyLong_FromLongLong Jan 9, 2022
@mdickinson mdickinson removed the skip issue label Jan 9, 2022
@mdickinson
Add news entry
e5e868e
@mdickinson mdickinson removed the skip news label Jan 9, 2022
mdickinson
mdickinson commented Jan 9, 2022
View reviewed changes
Comment thread Objects/longobject.c
}

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

@mdickinson mdickinson Jan 9, 2022

Copy link
Copy Markdown
Member Author

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.

All reactions
@markshannon

markshannon commented Jan 21, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon self-requested a review January 21, 2022 15:01
mdickinson
mdickinson commented Jan 22, 2022
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member Author

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.

All reactions
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson
Silence compiler warnings
d7c7fe0
@mdickinson

mdickinson commented Jan 22, 2022

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon markshannon self-assigned this Feb 28, 2022
@markshannon

markshannon commented Mar 1, 2022

Copy link
Copy Markdown
Member

Thanks

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon merged commit c60e6b6 into python:main Mar 1, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon Awaiting requested review from markshannon

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@markshannon markshannon

Labels

None yet

Projects

None yet

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.

5 participants

@mdickinson @erlend-aasland @markshannon @the-knights-who-say-ni @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.