Skip to content

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Feb 13, 2025

@iritkatriel iritkatriel marked this pull request as ready for review February 18, 2025 10:32
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, 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.

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
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.

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.

@@ -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
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
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
6 tasks
colesbury pushed a commit to colesbury/cpython that referenced this pull request Mar 20, 2025
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
sobolevn added a commit that referenced this pull request Aug 2, 2025
)

This is required so we would never have empty node bodies.
Refs #130087
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 2, 2025
…pythonGH-137318)

This is required so we would never have empty node bodies.
Refs pythonGH-130087
(cherry picked from commit b74f3bed51378896f2c7c720e505e87373e68c79)

Co-authored-by: sobolevn <mail@sobolevn.me>
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
…python#137318)

This is required so we would never have empty node bodies.
Refs python#130087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants