FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-85283: Fix Argument Clinic for md5 extension by vstinner · Pull Request #110976 · python/cpython · GitHub

/ cpython Public

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

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

Merged
vstinner merged 1 commit into from
Oct 17, 2023
+4 −5
Conversation 9 Commits 1 Checks 0 Files changed 2
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 .h  (1) .py  (1) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 2 additions & 2 deletions Modules/clinic/md5module.c.h
Show comments
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

AlexWaygood Oct 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)
):

vstinner Oct 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 :-)

AlexWaygood Oct 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 :-)

vstinner Oct 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vstinner Oct 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

AlexWaygood Oct 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vstinner Oct 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL