gh-85283: Fix Argument Clinic for md5 extension #110976
Conversation
Limited C API supports the defining class under some conditions.
vstinner
requested review from
erlend-aasland,
AlexWaygood and
tiran
as code owners
|
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's not 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)): |
There was a problem hiding this comment.
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:
| 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) | |
| ): |
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
But, sure, we can leave this for now :-)
I would suggest to run Black on the whole file, not just one a few lines.
| 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.
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.
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.