Repository navigation
gh-108494: Fix AC limited C API for kwargs - #108516
vstinner wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Fix the limited C API code path in Argument Clinic to parse "positional or keywords" arguments. Add an unit test in _testclinic_limited.
|
@erlend-aasland @AlexWaygood @serhiy-storchaka: Here is a fix for PyArg_ParseTupleAndKeywords(). It removes the unused argname_fmt in the PyArg_ParseTuple() code path. |
Sorry, something went wrong.
| """, indent=4)] | ||
| declarations = "" | ||
|
|
||
| else: |
There was a problem hiding this comment.
Does it work if requires_defining_class is true?
Sorry, something went wrong.
There was a problem hiding this comment.
No, it doesn't. This change is incomplete, it can be completed later. Or do you mean that an exception should be raised instead of an compilation error?
Sorry, something went wrong.
There was a problem hiding this comment.
Currently it can produce invalid code which is compiled. There are two opposite approaches:
- Raise an exception or ensure that the compiling the generated code will get a compilation error.
- Try to use the limited API if possible, but fallback to private API otherwise.
I am currently trying the second approach. It allows to re-generate all clinic code using the limited API and test that at least it works (I found many cases in which the current code does not generate working code). Please hold this PR, the current code is more suitable for this experiment. After testing it will be easy to switch to the first approach.
Sorry, something went wrong.
There was a problem hiding this comment.
A module targeting the limited C API cannot use the private API. It fails at build.
Sorry, something went wrong.
| # positional-only for the limited C API | ||
| flags = "METH_VARARGS" | ||
| elif clinic.limited_capi: | ||
| if not requires_defining_class and pos_only == len(parameters) - pseudo_args: |
There was a problem hiding this comment.
Does it work if pseudo_args is not zero?
Sorry, something went wrong.
There was a problem hiding this comment.
I have no idea. I don't know what are pseudo args.
Sorry, something went wrong.
| {parse_arguments})) | ||
| goto exit; | ||
| """, indent=4)] | ||
| declarations = "char* _keywords[] = {{{keywords_c} NULL}};" |
There was a problem hiding this comment.
| declarations = "char* _keywords[] = {{{keywords_c} NULL}};" | |
| declarations = "static char *_keywords[] = {{{keywords_c} NULL}};" |
Sorry, something went wrong.
There was a problem hiding this comment.
I don't reallly understand the difference between the two. The value is constant so the variable should not really exist on the stack, no?
Sorry, something went wrong.
There was a problem hiding this comment.
There is a difference. It is an array of pointers. If it is a local variable, it should fill with initial values every time the function is called. If it is a static variable, it is only filled with initial values at the program's start (or when the function is called first time).
Sorry, something went wrong.
|
Does your PR make this one useless, or should it be merged first? |
Sorry, something went wrong.
|
I think that it makes it useless, maybe except the test. But for testing I want to reuse existing tests, so the current Modules/_testclinic_limited.c will become obsolete too. |
Sorry, something went wrong.
|
Serhiy's PR got merged and it's way more complete, I close this PR: #108536 |
Sorry, something went wrong.
Fix the limited C API code path in Argument Clinic to parse "positional or keywords" arguments.
Add an unit test in _testclinic_limited.