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

Jump to bottom

gh-91102: Port 8-argument _warnings.warn_explicit to Argument Clinic #92891

Merged
merged 9 commits into from
Jul 20, 2022
Merged

gh-91102: Port 8-argument _warnings.warn_explicit to Argument Clinic #92891

merged 9 commits into from
Jul 20, 2022

Conversation

arhadthedev
Copy link
Member

Benefits:

  1. METH_VARARGS calling convention is replaced with a little faster METH_FASTCALL (no extra tuple creation)
  2. PyArg_ParseTupleAndKeywords call is replaced with _PyArg_UnpackKeywords and a tailored parser generated specially for the ported function.
  3. Stresses that warn_explicit() differentiates situations when its object argument is set to Py_None and NULL, while for other optional arguments there is no difference.

/cc @rhettinger as a person who helped me to learn to classify whether extra abstractions pay off with benefits. This case looks like they do.

Related issue: gh-91102.

@arhadthedev
Port 8-argument _warnings.warn_explicit to Argument Clinic
bfefa95 
The generated version:
- replaces METH_VARARGS with METH_FASTCALL
- replaces PyArg_ParseTupleAndKeywords with _PyArg_UnpackKeywords
- stresses that warn_explicit() differs object=Py_None and object=NULL
erlend-aasland
erlend-aasland requested changes May 17, 2022
View reviewed changes
Python/_warnings.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

kumaraditya303
kumaraditya303 reviewed Jun 24, 2022
View reviewed changes
Python/_warnings.c Outdated Show resolved Hide resolved
@arhadthedev
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

@arhadthedev arhadthedev mentioned this pull request Jun 24, 2022
Closed
@arhadthedev
Copy link
Member Author

Turning this PR draft until gh-94431 is merged. Without that fix, a proper renaming (module as _module) doesn't work.

@arhadthedev arhadthedev marked this pull request as draft July 1, 2022 10:30
@erlend-aasland
Copy link
Contributor

Turning this PR draft until gh-94431 is merged. Without that fix, a proper renaming (module as _module) doesn't work.

...given my proposed solution is an acceptable solution ;) Other core devs might not agree with me, so we might have to go with your workaround. IMO, gh-94431 is a good approach.

@erlend-aasland
Copy link
Contributor

AC is now updated with new functionality. Please apply the suggestion in order to restore the parameter name as it was.

@arhadthedev
Copy link
Member Author

@erlend-aasland Thank you for the fix applied; I renamed the second module as @kumaraditya303 suggested.

@arhadthedev arhadthedev marked this pull request as ready for review July 7, 2022 10:07
erlend-aasland
erlend-aasland approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM, but the NEWS entry is IMO too verbose. It should be sufficient to say that _warnings.warn_explicit is ported to AC, and nothing else. I don't think the nitty-gritty details are very interesting for the average user; for core devs, triagers, and other contributors, the benefits are obvious.

@erlend-aasland erlend-aasland merged commit 41e0585 into python:main Jul 20, 2022
@erlend-aasland
Copy link
Contributor

Thanks, Oleg and Kumar.

@arhadthedev arhadthedev deleted the warn_explicit-ac branch July 20, 2022 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arhadthedev @bedevere-bot @erlend-aasland @kumaraditya303