Uh oh!
There was an error while loading. Please reload this page.
gh-108444: Add PyLong_AsInt() public function - #108445
Conversation
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 |
| with self.assertRaises(OverflowError): | ||
| PyLong_AsInt(INT_MAX + 1) | ||
| # invalid type |
There was a problem hiding this comment.
Please add also tests for objects with __index__
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.
| 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/