◐ Shell
reader mode source ↗
Skip to content

gh-108494: Argument Clinic: fix support of Limited C API#108536

Merged
serhiy-storchaka merged 7 commits into
python:mainfrom
serhiy-storchaka:clinic-limited-capi
Aug 28, 2023
Merged

gh-108494: Argument Clinic: fix support of Limited C API#108536
serhiy-storchaka merged 7 commits into
python:mainfrom
serhiy-storchaka:clinic-limited-capi

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Aug 27, 2023

Copy link
Copy Markdown
Member

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 in test_clinic (as expected). _struct.c and winreg.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.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

Yet one issue: the TokenizerIter constructor is incompatible with PyArg_ParseTupleAndKeywords. I temporary worked it around by adding the default value for keyword-only extra_tokens parameter. Similar incompatibility was also a cause of failing two test_clinic tests. Resolving #74366 should fix this.

@vstinner

Copy link
Copy Markdown
Member

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.

@serhiy-storchaka

Copy link
Copy Markdown
Member Author

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.

@serhiy-storchaka serhiy-storchaka requested a review from a team as a code owner August 28, 2023 11:11
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I may have missed something, but this looks good to me. Thanks, Serhiy!

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

No complaints from me (though I'm not an expert on the limited C API)

@erlend-aasland erlend-aasland changed the title gh-108494: Argument Clinic: fix support of limited C API Aug 28, 2023
@serhiy-storchaka serhiy-storchaka merged commit bc5356b into python:main Aug 28, 2023
@serhiy-storchaka serhiy-storchaka deleted the clinic-limited-capi branch August 28, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants