GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-108494: Argument Clinic: fix support of Limited C API by serhiy-storchaka · Pull Request #108536 · python/cpython

Conversation

serhiy-storchaka commented Aug 27, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

Issue: Add support for the limited C API to Argument Clinic #108494

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka added skip news topic-argument-clinic labels

Aug 27, 2023

serhiy-storchaka requested a review from vstinner

August 27, 2023 10:42

serhiy-storchaka requested review from AlexWaygood, berkerpeksag, erlend-aasland and ethanfurman as code owners

August 27, 2023 10:42

bedevere-bot mentioned this pull request

Aug 27, 2023

Add support for the limited C API to Argument Clinic #108494

Closed

bedevere-bot added the awaiting core review label

Aug 27, 2023

serhiy-storchaka removed request for berkerpeksag and ethanfurman

August 27, 2023 10:43


          pythongh-108494: Argument Clinic: fix support of limited C API

serhiy-storchaka force-pushed the clinic-limited-capi branch from 952e38c to 1896045 Compare

August 27, 2023 10:47

serhiy-storchaka commented Aug 27, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka mentioned this pull request

Aug 27, 2023

gh-108494: Fix AC limited C API for kwargs #108516

Closed

serhiy-storchaka commented

Aug 27, 2023

View reviewed changes

Comment thread

Lib/test/test_call.py


		def test_varargs1min(self):
		msg = r"get expected at least 1 argument, got 0"
		msg = (r"get\(\) takes at least 1 argument \(0 given\)\|"

serhiy-storchaka Aug 27, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_sqlite/module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_tracemalloc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Aug 27, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented Aug 27, 2023

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.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented

Aug 27, 2023

View reviewed changes

Comment thread

Modules/clinic/_testclinic_limited.c.h


		arg = PyLong_AsInt(arg_);
		if (arg == -1 && PyErr_Occurred()) {
		if (!PyArg_Parse(arg_, "i:my_int_func", &arg)) {

serhiy-storchaka Aug 27, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

erlend-aasland reviewed

Aug 28, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 4 commits

August 28, 2023 12:34


          Update warnings.

2a535ce


          Autodetect unsupported combination of required and optional keyword-o…

06b40ab

…nly parameters.


          Explicitly mark broken converters.

4a77952


          Merge branch 'main' into clinic-limited-capi

serhiy-storchaka requested a review from a team as a code owner

August 28, 2023 11:11


          Revert unnecessary changes.

19e5ee2

erlend-aasland reviewed

Aug 28, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Tools/clinic/clinic.py

f20b03c

Co-authored-by: Erlend E. Aasland <[email protected]>

erlend-aasland reviewed

Aug 28, 2023

View reviewed changes

Comment thread

Modules/_struct.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland approved these changes

Aug 28, 2023

View reviewed changes

erlend-aasland left a comment

Copy link

Copy Markdown

Contributor

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Aug 28, 2023

AlexWaygood approved these changes

Aug 28, 2023

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland changed the title ~~gh-108494: Argument Clinic: fix support of limited C API~~ gh-108494: Argument Clinic: fix support of Limited C API

Aug 28, 2023

serhiy-storchaka merged commit bc5356b into python:main

Aug 28, 2023

bedevere-bot removed the awaiting merge label

Aug 28, 2023

serhiy-storchaka deleted the clinic-limited-capi branch

August 28, 2023 13:04

Reviewers

erlend-aasland

erlend-aasland approved these changes

AlexWaygood

AlexWaygood approved these changes

vstinner

Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-argument-clinic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

serhiy-storchaka commented Aug 27, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Aug 27, 2023

Uh oh!

serhiy-storchaka Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Aug 27, 2023

Uh oh!

serhiy-storchaka commented Aug 27, 2023

Uh oh!

serhiy-storchaka Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Aug 27, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.