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

gh-85283: Fix Argument Clinic for md5 extension #110976

Merged
merged 1 commit into from
Oct 17, 2023
Merged

gh-85283: Fix Argument Clinic for md5 extension #110976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Modules/clinic/md5module.c.h
View file
Open in desktop

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

5 changes: 2 additions & 3 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,7 @@ def parser_body(

fastcall = not new_or_init
limited_capi = clinic.limited_capi
if limited_capi and (requires_defining_class or pseudo_args or
if limited_capi and (pseudo_args or
(any(p.is_optional() for p in parameters) and
any(p.is_keyword_only() and not p.is_optional() for p in parameters)) or
any(c.broken_limited_capi for c in converters)):
Comment on lines +1202 to 1205
Copy link
Member

@AlexWaygood AlexWaygood Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know this formatting style predates your PR, but I find this quite hard to read, and I think the readability issue is probably at least in part why a bug crept in here. I'd find it easier to parse if we used a formatting style a little closer to what black might do:

Suggested change
if limited_capi and (pseudo_args or
(any(p.is_optional() for p in parameters) and
any(p.is_keyword_only() and not p.is_optional() for p in parameters)) or
any(c.broken_limited_capi for c in converters)):
if limited_capi and (
pseudo_args
or (any(p.is_optional() for p in parameters)
and any(p.is_keyword_only() and not p.is_optional() for p in parameters))
or any(c.broken_limited_capi for c in converters)
):

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to have a kind of consistency in terms of coding style. If this block is changed, many other lines should be changed. For example, the style in this file is to put the operator at the end of a line, not at the beginning.

I need this fix to unblock a large following change: PR #110966.

Once I landed these changes, feel free to run Black, or change the coding style manually :-)

Copy link
Member

Choose a reason for hiding this comment

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

I only comment about formatting here because I think the poor readability was a contributing factor to why a bug crept in to these lines. But, sure, we can leave this for now :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that it was a bug. IMO my change is wrong :-) But I am not sure... AC is complicated and has a small test suite. Maybe recent changes for limited C API already cover all cases. Maybe not.

I made the change because it prevented me to merge PR #110966: with this PR, md5 clinic code started again to use the internal C API which was wrong. I mean, it injected a pycore_modsupport.h which is incompatible with the limited C API, even if the include was not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, sure, we can leave this for now :-)

I would suggest to run Black on the whole file, not just one a few lines.

Expand Down Expand Up @@ -1642,12 +1642,11 @@ def parser_body(
declarations=declarations)


methoddef_cast_end = ""
if flags in ('METH_NOARGS', 'METH_O', 'METH_VARARGS'):
methoddef_cast = "(PyCFunction)"
methoddef_cast_end = ""
elif limited_capi:
methoddef_cast = "(PyCFunction)(void(*)(void))"
methoddef_cast_end = ""
Comment on lines +1645 to -1650
Copy link
Member

Choose a reason for hiding this comment

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

This is just a refactor, right? I weakly prefer the existing code. Even though it's more verbose, it has a nice symmetry between the branches: methoddef_cast_end is set on each branch

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I added methoddef_cast_end and I regret that I added it to all branches, since it's only needed in one branch. Yes, it's a minor coding style change. It doesn't change the code.

else:
methoddef_cast = "_PyCFunction_CAST("
methoddef_cast_end = ")"
Expand Down