gh-108494: Fix AC limited C API for kwargs#108516
gh-108494: Fix AC limited C API for kwargs#108516vstinner wants to merge 1 commit intopython:mainfrom
Conversation
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 |
| """, indent=4)] | ||
| declarations = "" | ||
|
|
||
| else: |
There was a problem hiding this comment.
Does it work if requires_defining_class is true?
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?
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.
There was a problem hiding this comment.
A module targeting the limited C API cannot use the private API. It fails at build.
| # 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?
There was a problem hiding this comment.
I have no idea. I don't know what are pseudo args.
| {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}};" |
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?
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).
|
See #108536. |
|
Does your PR make this one useless, or should it be merged first? |
|
I think that it makes it useless, maybe except the test. But for testing I want to reuse existing tests, so the current |
|
Serhiy's PR got merged and it's way more complete, I close this PR: #108536 |
3 participants
Fix the limited C API code path in Argument Clinic to parse "positional or keywords" arguments.
Add an unit test in _testclinic_limited.