-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-108494: Argument Clinic: fix support of Limited C API #108536
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-108494: Argument Clinic: fix support of Limited C API #108536
Conversation
952e38c
to
1896045
Compare
Yet one issue: the |
@@ -65,7 +65,8 @@ def test_varargs3(self): | |||
self.assertRaisesRegex(TypeError, msg, int.from_bytes, b'a', 'little', False) | |||
|
|||
def test_varargs1min(self): | |||
msg = r"get expected at least 1 argument, got 0" | |||
msg = (r"get\(\) takes at least 1 argument \(0 given\)|" |
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.
PyArg_ParseTuple()
produces slightly different error message.
I'm very excited by this change! I will try to review it soon. Would it be possible to write unit tests in _testclinic_limited and test_clinic, for unusual cases like deprecated parameters and (if possible) defining class? In my other PR, I wrote tests for basic positional or keywords parameters. |
As for tests, I think that we should run all (or almost all) clinic tests in two modes. Instead of adding new tests for the limited API or duplicating test code we should generate and build different modules from the same source with different options. But this is a different PR. |
@@ -37,8 +37,7 @@ my_int_func(PyObject *module, PyObject *arg_) | |||
int arg; | |||
int _return_value; | |||
|
|||
arg = PyLong_AsInt(arg_); | |||
if (arg == -1 && PyErr_Occurred()) { | |||
if (!PyArg_Parse(arg_, "i:my_int_func", &arg)) { |
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.
It looks like regression, but actually not all inlined parsing code is compatible with the limited C API. In following PR I will implement inlining correctly, only for converters compatible with the limited C API. It will be large but mostly boring change (adding new parameter and simple checks in tens of functions), so I did not include it here.
Co-authored-by: Erlend E. Aasland <[email protected]>
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.
I may have missed something, but this looks good to me. Thanks, Serhiy!
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.
No complaints from me (though I'm not an expert on the limited C API)
I tried to enable the use of the limited C API in Argument Clinic for all code. With this PR it generates working code, and all tests are passed, except the low-level vectorcall test in
test_call
and few tests intest_clinic
(as expected)._struct.c
andwinreg.c
use converters incompatible with the limited C API. I temporary blacklisted these files. I only tested on Linux, so there may be errors in Windows or macOS specific modules.