-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-108444: Add PyLong_AsInt() public function #108445
Conversation
@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().
Just last week I thought about this. Should we add also Should we add |
Having to check Maybe the API should:
I would prefer to not add too many functions. The |
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.
with self.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 resolved
Hide resolved
@@ -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` |
…OSS.rst Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
This API extends the list of problematic APIs listed in capi-workgroup/problems#1. How about a non-ambiguous return value? |
I suppose that the similar proportion should be in the third-party code. But they use |
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 -l
36 I'm not sure a handful of cases warrants the introduction of yet another ambiguous API. |
And what? Argument Clinic uses |
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. |
No, I mean that you can expect a huge number of use cases for |
Aha, sorry, I misunderstood you. Yes, that is a valid point. I withdraw my complain. |
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):
|
There's a new commit after the PR has been approved. @serhiy-storchaka: please review the changes made to this pull request. |
Yes, that makes sense. |
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 |
... or even better: |
I don't think this should be added to the stable ABI now. |
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. |
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. |
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. |
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/