Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Limited C API supports the defining class under some conditions.
I'm not sure how I managed PR#110967 with outdated Argument Clinic file (Modules/clinic/md5module.c.h). It seems like |
Ah, in fact, the file got regenerated, it's just that a warning was emitted: It'snot an error. The problem is that with PR#110966, it becomes an error. |
| 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)): |
AlexWaygoodOct 17, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 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:
| iflimited_capiand (pseudo_argsor | |
| (any(p.is_optional()forpinparameters)and | |
| any(p.is_keyword_only()andnotp.is_optional()forpinparameters))or | |
| any(c.broken_limited_capiforcinconverters)): | |
| iflimited_capiand ( | |
| pseudo_args | |
| or (any(p.is_optional()forpinparameters) | |
| andany(p.is_keyword_only()andnotp.is_optional()forpinparameters)) | |
| orany(c.broken_limited_capiforcinconverters) | |
| ): |
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 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 :-)
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 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 :-)
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 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 apycore_modsupport.h which is incompatible with the limited C API, even if the include was not used.
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.
But, sure, we can leave this for now :-)
I would suggest to run Black on the whole file, not just one a few lines.
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.
two nits:
| 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 = "" |
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.
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
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.
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.
Limited C API supports the defining class under some conditions.
Limited C API supports the defining class under some conditions.
Uh oh!
There was an error while loading.Please reload this page.
Limited C API supports the defining class under some conditions.