Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168

Merged
merged 21 commits into from
Jan 23, 2025
Merged

gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168

merged 21 commits into from
Jan 23, 2025
+8 −2

Conversation

srinivasreddy
Copy link
Contributor

@srinivasreddy srinivasreddy commented Jan 22, 2025 •
edited by bedevere-app bot
Loading

@srinivasreddy srinivasreddy changed the title Add Missing fast path in PyLong_From*() functions for compact integers gh-129149: Add Missing fast path in PyLong_From*() functions for compact integers Jan 22, 2025
@bedevere-app bedevere-app bot mentioned this pull request Jan 22, 2025
Open
@skirpichev

This comment was marked as resolved.

Sign in to view
@srinivasreddy srinivasreddy marked this pull request as ready for review January 22, 2025 09:06
@skirpichev skirpichev self-requested a review January 22, 2025 09:34
srinivasreddy and others added 2 commits January 22, 2025 15:16
@srinivasreddy @eendebakpt
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
c00ba8c 
…e-129149.wAYu43.rst

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@srinivasreddy @eendebakpt
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
2b5163f 
…e-129149.wAYu43.rst

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
WolframAlph
WolframAlph reviewed Jan 22, 2025
View reviewed changes
Objects/longobject.c Outdated Show resolved Hide resolved
skirpichev
skirpichev approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

LGTM, but I second on comment on declarations in *_FROM_SIGNED. Also, I don't think that PYLONG_FROM_UINT should be moved.

Objects/longobject.c Outdated Show resolved Hide resolved
WolframAlph
WolframAlph reviewed Jan 22, 2025
View reviewed changes
Objects/longobject.c Outdated Show resolved Hide resolved
picnixz
picnixz reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

If the code was only moved around and there's no new fast path, I would prefer to do it in a separate PR rather than in this one.

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
Objects/longobject.c Outdated Show resolved Hide resolved
@srinivasreddy srinivasreddy requested a review from picnixz January 23, 2025 06:22
@srinivasreddy srinivasreddy marked this pull request as draft January 23, 2025 06:45
@srinivasreddy srinivasreddy changed the title gh-129149: Add Missing fast path in PyLong_From*() functions for compact integers gh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integers Jan 23, 2025
@srinivasreddy srinivasreddy changed the title gh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integers gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers Jan 23, 2025
@srinivasreddy srinivasreddy marked this pull request as ready for review January 23, 2025 06:56
@srinivasreddy
Copy link
Contributor Author

srinivasreddy commented Jan 23, 2025 •
edited
Loading

Action plan for PRs.

  1. Add Missing fast path in PYLONG_FROM_UINT macro for compact integers gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168
  2. Refactor PyLong_FromSsize_t(), PyLong_FromLong() and PyLong_FromLongLong() using a macro similar to PYLONG_FROM_UINT to get rid of the repetitive code.

cc @skirpichev @picnixz

skirpichev
skirpichev reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

No, you misread me. Refactoring change (conversion PyLong_FromLongLong and PyLong_FromLong to macro) doesn't require a news entry. But this change - does.

@srinivasreddy
Revert "Remove blurb"
5a9e201 
This reverts commit af410e0.
@srinivasreddy
Copy link
Contributor Author

@skirpichev Done.

picnixz
picnixz reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Nit and LGTM.

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
@srinivasreddy @picnixz
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
1f81b87 
…e-129149.wAYu43.rst

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@srinivasreddy
Copy link
Contributor Author

@picnixz Done 👍🏾

picnixz
picnixz approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

This looks fine but I'll @vstinner have a look as I'm not very familiar with this performance part.

vstinner
vstinner approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
@srinivasreddy @vstinner
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
f4d74d2 
…e-129149.wAYu43.rst

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner vstinner enabled auto-merge (squash) January 23, 2025 14:36
@vstinner vstinner merged commit ab353b3 into python:main Jan 23, 2025
41 checks passed
@vstinner
Copy link
Member

Merged, thanks @srinivasreddy!

@srinivasreddy srinivasreddy deleted the gh-129149 branch January 23, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-eibl chris-eibl chris-eibl left review comments

@WolframAlph WolframAlph WolframAlph left review comments

@eendebakpt eendebakpt eendebakpt left review comments

@picnixz picnixz picnixz approved these changes

@skirpichev skirpichev skirpichev approved these changes

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@srinivasreddy @skirpichev @vstinner @eendebakpt @picnixz @WolframAlph @chris-eibl