gh-122943: Rework support of var-positional parameter in Argument Clinic - #122945
Conversation
|
@serhiy-storchaka, @erlend-aasland, I did port of #126064 on top of this pr. Diff follows (only Tools/clinic stuff included): Detailsdiff --git a/Tools/clinic/libclinic/clanguage.py b/Tools/clinic/libclinic/clanguage.py
index 32d2c045b0..32aba81ab8 100644
--- a/Tools/clinic/libclinic/clanguage.py
+++ b/Tools/clinic/libclinic/clanguage.py
@@ -15,7 +15,7 @@
Module, Class, Function, Parameter,
permute_optional_groups,
GETTER, SETTER, METHOD_INIT)
-from libclinic.converters import self_converter
+from libclinic.converters import defining_class_converter, self_converter
from libclinic.parse_args import ParseArgsCodeGen
if TYPE_CHECKING:
from libclinic.app import Clinic
@@ -396,6 +396,12 @@ def render_function(
first_optional = len(selfless)
positional = selfless and selfless[-1].is_positional_only()
has_option_groups = False
+ requires_defining_class = (len(selfless)
+ and isinstance(selfless[0].converter,
+ defining_class_converter))
+ pass_vararg_directly = (all(p.is_positional_only() or p.is_vararg()
+ for p in selfless)
+ and not requires_defining_class)
# offset i by -1 because first_optional needs to ignore self
for i, p in enumerate(parameters, -1):
@@ -404,6 +410,9 @@ def render_function(
if (i != -1) and (p.default is not unspecified):
first_optional = min(first_optional, i)
+ if p.is_vararg() and not pass_vararg_directly:
+ data.cleanup.append(f"Py_XDECREF({c.parser_name});")
+
# insert group variable
group = p.group
if last_group != group:
@@ -415,6 +424,11 @@ def render_function(
data.impl_parameters.append("int " + group_name)
has_option_groups = True
+ if p.is_vararg() and pass_vararg_directly:
+ data.impl_arguments.append('nvararg')
+ data.impl_parameters.append('Py_ssize_t nargs')
+ p.converter.type = 'PyObject *const *'
+
c.render(p, data)
if has_option_groups and (not positional):
diff --git a/Tools/clinic/libclinic/converter.py b/Tools/clinic/libclinic/converter.py
index 7d74331c73..86853bb4fb 100644
--- a/Tools/clinic/libclinic/converter.py
+++ b/Tools/clinic/libclinic/converter.py
@@ -296,10 +296,7 @@ def _render_non_self(
data.post_parsing.append('/* Post parse cleanup for ' + name + ' */\n' + post_parsing.rstrip() + '\n')
# cleanup
- if parameter.is_vararg():
- cleanup = f"Py_XDECREF({self.parser_name});"
- else:
- cleanup = self.cleanup()
+ cleanup = self.cleanup()
if cleanup:
data.cleanup.append('/* Cleanup for ' + name + ' */\n' + cleanup.rstrip() + "\n")
diff --git a/Tools/clinic/libclinic/parse_args.py b/Tools/clinic/libclinic/parse_args.py
index edf13e9f95..10bee51e09 100644
--- a/Tools/clinic/libclinic/parse_args.py
+++ b/Tools/clinic/libclinic/parse_args.py
@@ -524,6 +524,11 @@ def parse_pos_only(self) -> None:
parser_code = []
max_args = NO_VARARG if self.varpos else self.max_pos
+ if self.varpos:
+ self.declarations = f"Py_ssize_t nvararg = {nargs} - {self.max_pos};"
+ else:
+ self.declarations = ""
+
if self.limited_capi:
if nargs != 'nargs':
nargs_def = f'Py_ssize_t nargs = {nargs};'
@@ -594,7 +599,11 @@ def parse_pos_only(self) -> None:
if has_optional:
parser_code.append("skip_optional:")
if self.varpos:
- parser_code.append(libclinic.normalize_snippet(self._parse_vararg(), indent=4))
+ if min(self.pos_only, self.min_pos) >= self.max_pos:
+ start = f'args + {self.max_pos}' if self.max_pos else 'args'
+ parser_code.append(libclinic.normalize_snippet(f"{self.varpos.converter.parser_name} = {start};", indent=4))
+ else:
+ parser_code.append(libclinic.normalize_snippet(self._parse_vararg(), indent=4))
else:
for parameter in self.parameters:
parameter.converter.use_converter()
@@ -619,7 +628,7 @@ def parse_pos_only(self) -> None:
goto exit;
}}
""", indent=4)]
- self.parser_body(*parser_code)
+ self.parser_body(*parser_code, declarations=self.declarations)
def parse_general(self, clang: CLanguage) -> None:
parsearg: str | NoneI did basic tests for affected modules (e.g. test_math or test_set), run also test_clinic. Seems working. I admit, this looks as a temporary solution. On pros, reverting #126064 will introduce performance regressions, e.g. for math.gcd/lcm/hypot and in some set methods. |
|
This is not so easy. Well, I managed to resolve conflicts. This PR now reverts #126064 and #126235 changes, then add converters for var-positional parameter. The result is not so bad as I was afraid, but the PR is now larger. This is not the final result. More changes will follow. Note that the gc and _testclinic modules again use a tuple representation. Now |
To apply the changes in other way. Don't worry, there is no regression in the math module. But the generated code is now different, and the hand-written code is different, because parameter names and order are different. |
It's in set methods. |
|
Do we really need to revert the clinic input in |
|
gcmodule is better with a tuple. Test modules now contain tests for both convertors and also for no-fastapi. There is nothing redundant. |
Why is it "better" to revert the recently added optimisation? What about the math module changes? The set module changes? None of this seems needed at all. |
…nt Clinic (pythonGH-122945) Move creation of a tuple for var-positional parameter out of _PyArg_UnpackKeywordsWithVararg(). Merge _PyArg_UnpackKeywordsWithVararg() with _PyArg_UnpackKeywords(). Add a new parameter in _PyArg_UnpackKeywords(). The "parameters" and "converters" attributes of ParseArgsCodeGen no longer contain the var-positional parameter. It is now available as the "varpos" attribute. Optimize code generation for var-positional parameter and reuse the same generating code for functions with and without keyword parameters. Add special converters for var-positional parameter. "tuple" represents it as a Python tuple and "array" represents it as a continuous array of PyObject*. "object" is a temporary alias of "tuple".
…nt Clinic (pythonGH-122945) Move creation of a tuple for var-positional parameter out of _PyArg_UnpackKeywordsWithVararg(). Merge _PyArg_UnpackKeywordsWithVararg() with _PyArg_UnpackKeywords(). Add a new parameter in _PyArg_UnpackKeywords(). The "parameters" and "converters" attributes of ParseArgsCodeGen no longer contain the var-positional parameter. It is now available as the "varpos" attribute. Optimize code generation for var-positional parameter and reuse the same generating code for functions with and without keyword parameters. Add special converters for var-positional parameter. "tuple" represents it as a Python tuple and "array" represents it as a continuous array of PyObject*. "object" is a temporary alias of "tuple".

Move creation of a tuple for var-positional parameter out of _PyArg_UnpackKeywordsWithVararg().
Merge _PyArg_UnpackKeywordsWithVararg() with _PyArg_UnpackKeywords(). Add a new parameter in _PyArg_UnpackKeywords().
The "parameters" and "converters" attributes of ParseArgsCodeGen no longer contain the var-positional parameter. It is now available as the "varpos" attribute. Optimize code generation for var-positional parameter and reuse the same generating code for functions with and without keyword parameters.