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-130080: implement PEP 765 #130087

Merged
merged 22 commits into from
Mar 17, 2025
Merged

gh-130080: implement PEP 765 #130087

merged 22 commits into from
Mar 17, 2025

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Feb 13, 2025 •
edited
Loading

@iritkatriel iritkatriel requested a review from ncoghlan February 13, 2025 16:56
@bedevere-app bedevere-app bot mentioned this pull request Feb 13, 2025
Closed
@iritkatriel iritkatriel marked this pull request as draft February 14, 2025 00:26
@iritkatriel iritkatriel marked this pull request as ready for review February 18, 2025 10:32
markshannon
markshannon requested changes Mar 3, 2025
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, but I'm not sure I entirely trust the overwrite/save/overwrite logic.

Saving, overwriting and then overwriting again seems an error prone way to maintain a stack of the current context. ControlFlowInFinallyState can fit in a single byte and the depth of try-finally is limited to 20 (CO_MAXBLOCKS) so a stack will only take 20 bytes or so.

@bedevere-app
Copy link

bedevere-app bot commented Mar 3, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

markshannon
markshannon requested changes Mar 6, 2025
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Looks good in general.
The two uses of CO_MAXBLOCKS differ, so will need to be aligned.

Could also use a test or two for deeply nested functions with loops and try-finallys.

@bedevere-app
Copy link

bedevere-app bot commented Mar 6, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

tomasr8
tomasr8 reviewed Mar 15, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

I noticed I had some pending comments which I forgot to send.. 😄

iritkatriel and others added 4 commits March 15, 2025 18:27
@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Mar 15, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon March 15, 2025 20:45
markshannon
markshannon approved these changes Mar 17, 2025
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

There are two unused fields in _PyASTOptimizeState that need removing, otherwise looks good.

Python/ast_opt.c Outdated
int optimize;
int ff_features;
int syntax_check_only;

int recursion_depth; /* current recursion depth */
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere (same for recursion_limit)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think they were removed in the const folding PRs after I created this one.

picnixz
picnixz reviewed Mar 17, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Just some docs comments.

Doc/whatsnew/3.14.rst
@@ -68,6 +68,7 @@ Summary -- release highlights
* :ref:`PEP 741: Python Configuration C API <whatsnew314-pep741>`
* :ref:`PEP 761: Discontinuation of PGP signatures <whatsnew314-pep761>`
* :ref:`A new type of interpreter <whatsnew314-tail-call>`
* :ref:`PEP 765: Disallow return/break/continue that exit a finally block <whatsnew314-pep765>`
Copy link
Member

Choose a reason for hiding this comment

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

For the reader, I think it may be better to have the PEP bullet points grouped.

iritkatriel and others added 2 commits March 17, 2025 20:12
@iritkatriel @picnixz
Apply suggestions from code review
a00f297 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@iritkatriel @picnixz
Update Doc/whatsnew/3.14.rst
459ccf0 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@iritkatriel iritkatriel enabled auto-merge (squash) March 17, 2025 20:30
@iritkatriel iritkatriel merged commit ffc2f1d into python:main Mar 17, 2025
42 checks passed
@AA-Turner AA-Turner mentioned this pull request Mar 18, 2025
Merged
6 tasks
colesbury pushed a commit to colesbury/cpython that referenced this pull request Mar 20, 2025
@iritkatriel @colesbury
pythongh-130080: implement PEP 765 (python#130087)
eb47811
@iritkatriel iritkatriel mentioned this pull request Mar 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomasr8 tomasr8 tomasr8 left review comments

@picnixz picnixz picnixz left review comments

@WolframAlph WolframAlph WolframAlph left review comments

@markshannon markshannon markshannon approved these changes

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@isidentical isidentical Awaiting requested review from isidentical isidentical is a code owner

@Eclips4 Eclips4 Awaiting requested review from Eclips4 Eclips4 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

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

Successfully merging this pull request may close these issues.
5 participants
@iritkatriel @tomasr8 @markshannon @picnixz @WolframAlph