Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-108444: Add PyLong_AsInt() public function#108445
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Aug 24, 2023 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Aug 24, 2023
@serhiy-storchaka: You added the function 10 years, what do you think of making it public now? |
* Rename _PyLong_AsInt() to PyLong_AsInt(). * Add documentation. * Add test. * For now, keep _PyLong_AsInt() as an alias to PyLong_AsInt().
serhiy-storchaka commented Aug 24, 2023
Just last week I thought about this. Should we add also Should we add |
vstinner commented Aug 24, 2023
Having to check Maybe the API should:
I would prefer to not add too many functions. The |
serhiy-storchaka left a comment
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.
LGTM.
| withself.assertRaises(OverflowError): | ||
| PyLong_AsInt(INT_MAX+1) | ||
| # invalid type |
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.
Please add also tests for objects with __index__
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.
Ok, I added a test
Misc/NEWS.d/next/C API/2023-08-24-20-08-02.gh-issue-108014.20DOSS.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,4 @@ | |||
| Add :c:func:`PyLong_AsInt` function: similar to :c:func:`PyLong_AsLong`, but | |||
| store the result in a C :c:expr:`int` instead of a C :c:expr:`long`. | |||
| Previously, it was known as the the private function :c:func:`!_PyLong_AsInt` | |||
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.
| Previously, it was known as the the private function :c:func:`!_PyLong_AsInt` | |
| Previously, it was known as the private function :c:func:`!_PyLong_AsInt` |
Uh oh!
There was an error while loading. Please reload this page.
…OSS.rst Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
erlend-aasland commented Aug 24, 2023
This API extends the list of problematic APIs listed in capi-workgroup/problems#1. How about a non-ambiguous return value? |
erlend-aasland commented Aug 24, 2023
serhiy-storchaka commented Aug 24, 2023
I suppose that the similar proportion should be in the third-party code. But they use |
erlend-aasland commented Aug 24, 2023
That number is true, but the majority of it comes from Argument Clinic. If you subtract generated code, you will see that the actual number is less then a tenth of 420: $ git grep _PyLong_AsInt | grep -v clinic | wc -l36I'm not sure a handful of cases warrants the introduction of yet another ambiguous API. |
serhiy-storchaka commented Aug 24, 2023
And what? Argument Clinic uses |
erlend-aasland commented Aug 24, 2023
The point is that there is not 420 occurrences of You wrote:
I assumed you meant that it was not easy to use a different API because of the huge number of occurrences (420). My interpretation was that since the actual number of occurrences that need replacement is significantly lower, your argument of it being hard to replace is moot. |
serhiy-storchaka commented Aug 24, 2023
No, I mean that you can expect a huge number of use cases for |
erlend-aasland commented Aug 24, 2023
Aha, sorry, I misunderstood you. Yes, that is a valid point. I withdraw my complain. |
vstinner commented Aug 24, 2023
I'm well aware of PyLong_AsLong() API issue. But if we fix the API, I would prefer to fix the whole PyLong API family, not a single function. It would be surprising to have a single function with a similar name, but a different API. By the way, the private Affected projects (9):
|
bedevere-bot commented Aug 24, 2023
There's a new commit after the PR has been approved. @serhiy-storchaka: please review the changes made to this pull request. |
erlend-aasland commented Aug 24, 2023
Yes, that makes sense. |
vstinner commented Aug 24, 2023
The function is more if it's worth it to propose a new set of PyLong_AsLong-like function which avoid having to check PyErr_Occurred(): API like |
erlend-aasland commented Aug 25, 2023
... or even better: |
encukou commented Aug 31, 2023
I don't think this should be added to the stable ABI now. |
vstinner commented Aug 31, 2023
The private _PyLong_AsInt() function seems to be pretty commonly used in the wild, so adding it as public API helps people to migrate to this new public C API. I would like to add it the limited C API to be able to use it in C code generated by Argument Clinic which seems to like a lot PyLong_AsInt(). For example, in the main branch, there are 70 For now, there is no concrete plan to deprecate PyLong_AsLong() nor replacement. |
serhiy-storchaka commented Aug 31, 2023
Is there a way to gather some reliable statistics about non-public functions used in third-party code? If they are used in a number of non-toy projects, we should consider to provide a public API to satisfy the need. |
encukou commented Aug 31, 2023
I'm on board with that. But we should also use these opportunities to provide better API. Private functions can get away with being hacky. |
vstinner commented Aug 31, 2023
Known issues of PyLong_AsLong() were discussed in previous comments and it was decided to add PyLong_AsInt() as it is anyway. |
📚 Documentation preview 📚: https://cpython-previews--108445.org.readthedocs.build/