GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-85283: Fix Argument Clinic for md5 extension by vstinner · Pull Request #110976 · python/cpython

Conversation

vstinner commented Oct 17, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Limited C API supports the defining class under some conditions.

Issue: [C API] Convert a few stdlib extensions to the limited C API (PEP 384) #85283

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-85283: Fix Argument Clinic for md5 extension

e50946f

Limited C API supports the defining class under some conditions.

vstinner added the skip news label

Oct 17, 2023

vstinner requested review from AlexWaygood, erlend-aasland and tiran as code owners

October 17, 2023 11:19

bedevere-app Bot mentioned this pull request

Oct 17, 2023

[C API] Convert a few stdlib extensions to the limited C API (PEP 384) #85283

Closed

bedevere-app Bot added the awaiting core review label

Oct 17, 2023

vstinner commented Oct 17, 2023

Copy link

Copy Markdown

Member Author

I'm not sure how I managed PR #110967 with outdated Argument Clinic file (Modules/clinic/md5module.c.h). It seems like make clinic failed and I didn't noticed that it failed! Moreover, I expected "Check if generated files are up to date" does run make clinic but apparently it does not!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Oct 17, 2023

Copy link

Copy Markdown

Member Author

I'm not sure how I managed PR #110967 with outdated Argument Clinic file (Modules/clinic/md5module.c.h).

Ah, in fact, the file got regenerated, it's just that a warning was emitted:

Warning in file 'Modules/md5module.c' on line 114:
Function MD5Type.copy cannot use limited C API

It's not an error. The problem is that with PR #110966, it becomes an error.

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner enabled auto-merge (squash)

October 17, 2023 11:34

AlexWaygood reviewed

Oct 17, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py

Comment on lines +1202 to 1205

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

AlexWaygood Oct 17, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner Oct 17, 2023

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood Oct 17, 2023

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner Oct 17, 2023

Copy link

Copy Markdown

Member Author

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 a pycore_modsupport.h which is incompatible with the limited C API, even if the include was not used.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 17, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood reviewed

Oct 17, 2023

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood disabled auto-merge

October 17, 2023 11:39

AlexWaygood reviewed

Oct 17, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py

Comment on lines +1645 to -1650

		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 = ""

AlexWaygood Oct 17, 2023

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner Oct 17, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner merged commit 054f496 into python:main

Oct 17, 2023

vstinner deleted the fix_md5_clinic branch

October 17, 2023 11:46

bedevere-app Bot removed the awaiting core review label

Oct 17, 2023

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-85283: Fix Argument Clinic for md5 extension (python#110976)

3c0a3cd

Limited C API supports the defining class under some conditions.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-85283: Fix Argument Clinic for md5 extension (python#110976)

c54f498

Limited C API supports the defining class under some conditions.

Reviewers

AlexWaygood

AlexWaygood left review comments

erlend-aasland

Awaiting requested review from erlend-aasland

erlend-aasland is a code owner

tiran

Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

vstinner commented Oct 17, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 17, 2023

Uh oh!

vstinner commented Oct 17, 2023

Uh oh!

AlexWaygood Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vstinner commented Oct 17, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood Oct 17, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.