Skip to content

Comments

gh-116560: Add PyLong_GetSign() public function#116561

Merged
vstinner merged 18 commits intopython:mainfrom
skirpichev:PyLong_Sign-116560
Jun 3, 2024
Merged

gh-116560: Add PyLong_GetSign() public function#116561
vstinner merged 18 commits intopython:mainfrom
skirpichev:PyLong_Sign-116560

Conversation

@skirpichev
Copy link
Member

@skirpichev skirpichev commented Mar 10, 2024 •
edited
Loading

PyLong_Sign() now is a wrapper to _PyLong_Sign(): argument checking added, also documentation and tests

Notes: Some PyLong_As* functions fallback to __index__() call, some not. C.f. PyLong_AsLong() vs PyLong_AsSsize_t(). Not sure if this is not a bug, but second was chosen.

📚 Documentation preview 📚: https://cpython-previews--116561.org.readthedocs.build/

@skirpichev
pythongh-116560: Add PyLong_Sign() public function
cb4af47 
* rename _PyLong_Sign() to PyLong_Sign()
* add argument checking, documentation and tests
* keep _PyLong_Sign() as an alias to PyLong_Sign()
@bedevere-app bedevere-app bot mentioned this pull request Mar 10, 2024
Closed
@skirpichev skirpichev requested a review from Eclips4 March 11, 2024 02:44
@Eclips4 Eclips4 requested review from encukou and vstinner March 11, 2024 05:23
vstinner
vstinner reviewed Mar 11, 2024
View reviewed changes
@skirpichev skirpichev requested a review from vstinner March 11, 2024 11:12
vstinner
vstinner reviewed Mar 11, 2024
View reviewed changes
@vstinner vstinner mentioned this pull request Mar 11, 2024
Closed
@vstinner
Copy link
Member

vstinner commented Mar 11, 2024

I created capi-workgroup/decisions#19 for this API.

@vstinner
Copy link
Member

vstinner commented Jun 1, 2024

I created capi-workgroup/decisions#19 for this API.

The SC decided to adopt this API: capi-workgroup/decisions#19 (comment)

@skirpichev: Would you mind to update your PR for this API?

@skirpichev skirpichev requested a review from vstinner June 2, 2024 02:48
@skirpichev
Copy link
Member Author

skirpichev commented Jun 2, 2024

@vstinner, I think it's already in sync with that version. Sphinx docs use slightly different wording. Let me know if you prefer variant from capi-workgroup/decisions#19 (comment).

vstinner
vstinner reviewed Jun 2, 2024
View reviewed changes
@vstinner
Copy link
Member

vstinner commented Jun 2, 2024

@vstinner, I think it's already in sync with that version.

Well, the C API WG prefers the name PyLong_GetSign(). Can you please rename the function?

vstinner
vstinner reviewed Jun 2, 2024
View reviewed changes
@vstinner vstinner changed the title gh-116560: Add PyLong_Sign() public function gh-116560: Add PyLong_GetSign() public function Jun 2, 2024
vstinner
vstinner reviewed Jun 2, 2024
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.

Thanks for the update. The overall change LGTM, but I have some more comments.

skirpichev and others added 2 commits June 3, 2024 04:33
vstinner
vstinner approved these changes Jun 3, 2024
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 if you modify the doc as suggested :-)

skirpichev
skirpichev commented Jun 3, 2024
View reviewed changes
@vstinner @skirpichev
Update Doc/c-api/long.rst
89a07fe 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@vstinner vstinner enabled auto-merge (squash) June 3, 2024 07:48
@skirpichev
Copy link
Member Author

skirpichev commented Jun 3, 2024

Thanks for review and patience:)

auto-merge was automatically disabled June 3, 2024 07:55

Head branch was pushed to by a user without write access

devdanzin
devdanzin reviewed Jun 3, 2024
View reviewed changes
skirpichev
skirpichev commented Jun 3, 2024
View reviewed changes
@vstinner vstinner merged commit 61d3ab3 into python:main Jun 3, 2024
@vstinner
Copy link
Member

vstinner commented Jun 3, 2024

Merged, thanks.

@vstinner
Copy link
Member

vstinner commented Jun 3, 2024

Function added to pythoncapi-compat: python/pythoncapi-compat@18d1df7

@skirpichev skirpichev deleted the PyLong_Sign-116560 branch June 3, 2024 13:36
@skirpichev skirpichev mentioned this pull request Jun 3, 2024
Closed
mliezun pushed a commit to mliezun/cpython that referenced this pull request Jun 3, 2024
@skirpichev @vstinner @mliezun
pythongh-116560: Add PyLong_GetSign() public function (python#116561)
63dc89a 
Co-authored-by: Victor Stinner <vstinner@python.org>
barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
@skirpichev @vstinner @barneygale
pythongh-116560: Add PyLong_GetSign() public function (python#116561)
204a262 
Co-authored-by: Victor Stinner <vstinner@python.org>
@hugovk hugovk mentioned this pull request Jul 8, 2024
Merged
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@skirpichev @vstinner @noahbkim
pythongh-116560: Add PyLong_GetSign() public function (python#116561)
aaf27b6 
Co-authored-by: Victor Stinner <vstinner@python.org>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@skirpichev @vstinner @estyxx
pythongh-116560: Add PyLong_GetSign() public function (python#116561)
8b4014b 
Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devdanzin devdanzin devdanzin left review comments

@vstinner vstinner vstinner approved these changes

@Eclips4 Eclips4 Awaiting requested review from Eclips4

@encukou encukou Awaiting requested review from encukou

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skirpichev @vstinner @devdanzin @Eclips4