gh-108494: Argument Clinic: inline parsing code for positional-only parameters in the limited C API#108622
Conversation
Sorry, something went wrong.
serhiy-storchaka
requested review from
a team,
AlexWaygood,
erlend-aasland and
gpshead
as code owners
August 29, 2023 10:04
serhiy-storchaka
force-pushed
the
clinic-limited-capi-inline
branch
from
1cef242 to
31238a6
Compare
August 29, 2023 10:25
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
This PR avoids private _PyArg_BadArgument() and _PyArg_CheckPositional() functions in the limited C API, and generate similar code for that. What about adding these functions to the limited C API instead?
The advantage of your approach is that we can target limited C API older than version 3.13 which is appealing. But is it worth it?
Sorry, something went wrong.
|
I don't have the bandwith to review the generated C param parsing code. I guess Victor does a better job of reviewing that, than me. If Alex is fine with the clinic.py changes, you are good to go. BTW, I really like the introduction of the |
Sorry, something went wrong.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. The overall change LGTM. clinic.py coding style makes reviews really complicated :-( I prefer testing fastcall than new_or_init, that's a nice improvment.
Sorry, something went wrong.
gpshead
left a comment
gpshead
left a comment
There was a problem hiding this comment.
nice work! if there are bugs lurking in here (it is hard to visually spot everything in this kind of large clinic.py change) i'm sure they'll show themselves in generated code behaviors that can be fixed up later.
Sorry, something went wrong.
|
There's a new commit after the PR has been approved. @gpshead, @vstinner, @erlend-aasland, @AlexWaygood: please review the changes made to this pull request. |
Sorry, something went wrong.
bedevere-bot
requested review from
AlexWaygood,
erlend-aasland,
gpshead and
vstinner
September 3, 2023 14:28
Reviewers
6 participants
It fixes some code which uses private converters, like
_PyLong_UnsignedShort_Converter. Inlined code only uses the limited C API.But it still includes internal headers. It is difficult to not include them, because they are needed when the parsing code is not inlined, and this depends not only on the specified converter, but on other converters and other used features.
There is a behavior change: converters like
_PyLong_UnsignedShort_Converterraise ValueError for negative integer, but the inlined code raise OverflowError. It is very difficult to detect negative integer in the limited C API. It can be fixed by changing exception inPyLong_AsUnsignedLong(see #74019).I also reorganized the code for keyword parameters, so it will be easier to enable inlining once we add a public version of
_PyArg_UnpackKeywordsin the limited C API, which is the only stopper.All tests (except one) are passed with using the limited C API (if possible).