Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-116646, AC: Add CConverter.use_converter() method #116793

Merged
merged 2 commits into from Mar 14, 2024
Merged

gh-116646, AC: Add CConverter.use_converter() method #116793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0dfaba8
gh-116646, AC: Add CConverter.use_converter() method
vstinner Mar 14, 2024
c4551c8
Fix mypy: add type annotations
vstinner Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions Modules/clinic/_ssl.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 35 additions & 6 deletions Tools/clinic/clinic.py
View file
Open in desktop
Expand Up @@ -884,6 +884,7 @@ def parser_body(
displayname = parameters[0].get_displayname(0)
parsearg = converters[0].parse_arg(argname, displayname, limited_capi=limited_capi)
if parsearg is None:
converters[0].use_converter()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? If I remove the line there is no change if I regen clinic.

Copy link
Contributor

Choose a reason for hiding this comment

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

$ git grep "\<PyArg_Parse\>" **/clinic/*
Modules/clinic/_cursesmodule.c.h:    if (!PyArg_Parse(arg, "y:putp", &string)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "" F_HANDLE ":CloseHandle", &handle)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "I:ExitProcess", &ExitCode)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "" F_HANDLE ":GetExitCodeProcess", &process)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "" F_HANDLE ":GetModuleFileName", &module_handle)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "k:GetStdHandle", &std_handle)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "" F_POINTER ":UnmapViewOfFile", &address)) {
Modules/clinic/_winapi.c.h:    if (!PyArg_Parse(arg, "" F_POINTER ":VirtualQuerySize", &address)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_PID ":sched_getscheduler", &pid)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_PID ":sched_getparam", &pid)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_PID ":sched_rr_get_interval", &pid)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_PID ":sched_getaffinity", &pid)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_PID ":getsid", &pid)) {
Modules/clinic/posixmodule.c.h:    if (!PyArg_Parse(arg, "" _Py_PARSE_INTPTR ":get_handle_inheritable", &handle)) {
Modules/clinic/unicodedata.c.h:    if (!PyArg_Parse(arg, "s#:lookup", &name, &name_length)) {
Tools/clinic/clinic.py:                        if (!PyArg_Parse(%s, "{format_units}:{name}", {parse_arguments})) {{

Copy link
Member Author

Choose a reason for hiding this comment

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

use_converter() is more for converters using O& + callback function. I don't think that currently, the Python code base goes to this code path with a O& converter which emits an #include.

use_converter() is mainly needed to generate #include.

Copy link
Contributor

Choose a reason for hiding this comment

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

use_converter() is mainly needed to generate #include.

IMO, it would be better if this was done implicitly rather than explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be nice, but I failed to find a way to distinguish when code to parse arguments is "inlined" vs O& converters. The use_converter() method is called explicitly, but it only has to be called in a few places.

This PR removed an unused #include in Modules/clinic/_ssl.c.h.

parsearg = """
if (!PyArg_Parse(%s, "{format_units}:{name}", {parse_arguments})) {{
goto exit;
Expand Down Expand Up @@ -1016,6 +1017,9 @@ def parser_body(
if has_optional:
parser_code.append("skip_optional:")
else:
for parameter in parameters:
parameter.converter.use_converter()
Comment on lines +1020 to +1021
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not this belong to the if fastcall: scope in L1025?

Copy link
Member Author

Choose a reason for hiding this comment

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

The 3 code path below use {parse_arguments} and so use converters, no? I can be wrong, I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. I think the added use_converter method and the refactoring makes it hard to understand what's happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

The last path uses PyArg_ParseTuple, so no converters are used, right?


if limited_capi:
fastcall = False
if fastcall:
Expand Down Expand Up @@ -1184,6 +1188,9 @@ def parser_body(
if add_label:
parser_code.append("%s:" % add_label)
else:
for parameter in parameters:
parameter.converter.use_converter()

declarations = declare_parser(f, clinic=clinic,
hasformat=True,
limited_capi=limited_capi)
Expand Down Expand Up @@ -3155,8 +3162,13 @@ def format_code(self, fmt: str, *,
fmt = fmt.replace('{bad_argument2}', bad_argument2)
return fmt.format(argname=argname, paramname=self.parser_name, **kwargs)

def use_converter(self) -> None:
"""Method called when self.converter is used to parse an argument."""
pass

def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> str | None:
if self.format_unit == 'O&':
self.use_converter()
return self.format_code("""
if (!{converter}({argname}, &{paramname})) {{{{
goto exit;
Expand Down Expand Up @@ -3435,6 +3447,9 @@ def converter_init(self, *, bitwise: bool = False) -> None:
self.format_unit = 'H'
else:
self.converter = '_PyLong_UnsignedShort_Converter'

def use_converter(self) -> None:
if self.converter == '_PyLong_UnsignedShort_Converter':
self.add_include('pycore_long.h',
'_PyLong_UnsignedShort_Converter()')

Expand Down Expand Up @@ -3519,6 +3534,9 @@ def converter_init(self, *, bitwise: bool = False) -> None:
self.format_unit = 'I'
else:
self.converter = '_PyLong_UnsignedInt_Converter'

def use_converter(self) -> None:
if self.converter == '_PyLong_UnsignedInt_Converter':
self.add_include('pycore_long.h',
'_PyLong_UnsignedInt_Converter()')

Expand Down Expand Up @@ -3577,6 +3595,9 @@ def converter_init(self, *, bitwise: bool = False) -> None:
self.format_unit = 'k'
else:
self.converter = '_PyLong_UnsignedLong_Converter'

def use_converter(self) -> None:
if self.converter == '_PyLong_UnsignedLong_Converter':
self.add_include('pycore_long.h',
'_PyLong_UnsignedLong_Converter()')

Expand Down Expand Up @@ -3630,6 +3651,9 @@ def converter_init(self, *, bitwise: bool = False) -> None:
self.format_unit = 'K'
else:
self.converter = '_PyLong_UnsignedLongLong_Converter'

def use_converter(self) -> None:
if self.converter == '_PyLong_UnsignedLongLong_Converter':
self.add_include('pycore_long.h',
'_PyLong_UnsignedLongLong_Converter()')

Expand Down Expand Up @@ -3664,20 +3688,23 @@ def converter_init(self, *, accept: TypeSet = {int}) -> None:
if accept == {int}:
self.format_unit = 'n'
self.default_type = int
self.add_include('pycore_abstract.h', '_PyNumber_Index()')
elif accept == {int, NoneType}:
self.converter = '_Py_convert_optional_to_ssize_t'
self.add_include('pycore_abstract.h',
'_Py_convert_optional_to_ssize_t()')
else:
fail(f"Py_ssize_t_converter: illegal 'accept' argument {accept!r}")

def use_converter(self) -> None:
if self.converter == '_Py_convert_optional_to_ssize_t':
self.add_include('pycore_abstract.h',
'_Py_convert_optional_to_ssize_t()')

def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> str | None:
if self.format_unit == 'n':
if limited_capi:
PyNumber_Index = 'PyNumber_Index'
else:
PyNumber_Index = '_PyNumber_Index'
self.add_include('pycore_abstract.h', '_PyNumber_Index()')
return self.format_code("""
{{{{
Py_ssize_t ival = -1;
Expand Down Expand Up @@ -3771,7 +3798,7 @@ class size_t_converter(CConverter):
converter = '_PyLong_Size_t_Converter'
c_ignored_default = "0"

def converter_init(self, *, accept: TypeSet = {int, NoneType}) -> None:
def use_converter(self) -> None:
self.add_include('pycore_long.h',
'_PyLong_Size_t_Converter()')

Expand Down Expand Up @@ -3800,6 +3827,10 @@ class fildes_converter(CConverter):
type = 'int'
converter = '_PyLong_FileDescriptor_Converter'

def use_converter(self) -> None:
self.add_include('pycore_fileutils.h',
'_PyLong_FileDescriptor_Converter()')

def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> str | None:
if limited_capi:
return self.format_code("""
Expand All @@ -3810,8 +3841,6 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
""",
argname=argname)
else:
self.add_include('pycore_fileutils.h',
'_PyLong_FileDescriptor_Converter()')
return super().parse_arg(argname, displayname, limited_capi=limited_capi)


Expand Down