-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
gh-116646, AC: Add CConverter.use_converter() method #116793
Conversation
Only add includes when the converter is effectively used.
This change is needed to modify Otherwise, we would have to inject |
This comment was marked as off-topic.
This comment was marked as off-topic.
for parameter in parameters: | ||
parameter.converter.use_converter() |
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.
Should not this belong to the if fastcall:
scope in L1025?
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.
The 3 code path below use {parse_arguments}
and so use converters, no? I can be wrong, I'm not sure.
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 don't know. I think the added use_converter
method and the refactoring makes it hard to understand what's happening.
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.
The last path uses PyArg_ParseTuple
, so no converters are used, right?
@@ -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() |
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.
Is this needed? If I remove the line there is no change if I regen clinic.
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.
$ 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})) {{
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.
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
.
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.
use_converter()
is mainly needed to generate#include
.
IMO, it would be better if this was done implicitly rather than explicitly.
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.
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
.
…6793) Only add includes when the converter is effectively used.
…6793) Only add includes when the converter is effectively used.
…6793) Only add includes when the converter is effectively used.
Only add includes when the converter is effectively used.