◐ Shell
reader mode source ↗
Skip to content

bpo-29882: Fix portability bug introduced in GH-30774#30794

Merged
mdickinson merged 2 commits into
python:mainfrom
mdickinson:fix-popcount-portability
Jan 23, 2022
Merged

bpo-29882: Fix portability bug introduced in GH-30774#30794
mdickinson merged 2 commits into
python:mainfrom
mdickinson:fix-popcount-portability

Conversation

@mdickinson

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

Copy link
Copy Markdown
Member

This PR fixes a portability bug in _Py_popcount32 that was introduced in GH-30774, and adds a comment explaining why the final line of the function is delicate.

Prior discussions:

https://bugs.python.org/issue29882

@mdickinson mdickinson requested a review from vstinner January 22, 2022 15:26
@mdickinson mdickinson mentioned this pull request Jan 22, 2022
Merged
@vstinner

vstinner commented Jan 22, 2022

Copy link
Copy Markdown
Member

Can you please add a test for 2**28 + 1 in _testinternalcapi.test_popcount()?

ref: #30774 (comment)

@mdickinson

mdickinson commented Jan 22, 2022

Copy link
Copy Markdown
Member Author

Can you please add a test for 2**28 + 1 in _testinternalcapi.test_popcount()?

Sure, will do. Though note that there's really nothing at all special about that value: any input value larger than 255 will give the wrong result under the code currently in main, on a machine with 64-bit int.

vstinner
vstinner approved these changes Jan 22, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Adding (uint32_t) cast and the added test LGTM. I didn't review the long comment ;-)

tim-one
tim-one approved these changes Jan 22, 2022
View reviewed changes

@tim-one tim-one left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Looks fine to me!

@mdickinson mdickinson merged commit 83a0ef2 into python:main Jan 23, 2022
@bedevere-bot

bedevere-bot commented Jan 23, 2022

Copy link
Copy Markdown

@mdickinson: Please replace # with GH- in the commit message next time. Thanks!

@mdickinson mdickinson deleted the fix-popcount-portability branch January 23, 2022 09:59
@niklasf niklasf mannequin mentioned this pull request Nov 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@tim-one tim-one tim-one approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mdickinson @vstinner @bedevere-bot @tim-one