Skip to content

gh-126835: Rename AST optimization related stuff after moving const folding to the peephole optimizier #131830

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
Merged
merged 11 commits into from
May 4, 2025
Merged

gh-126835: Rename AST optimization related stuff after moving const folding to the peephole optimizier #131830

merged 11 commits into from
May 4, 2025

Conversation

WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Mar 28, 2025 •
edited by bedevere-app bot
Loading

@WolframAlph WolframAlph marked this pull request as draft March 28, 2025 11:34
@bedevere-app bedevere-app bot mentioned this pull request Mar 28, 2025
Open
@hugovk hugovk removed their request for review March 28, 2025 13:05
@WolframAlph WolframAlph changed the title gh-126835: Rename ast_opt.c -> ast_process.c gh-126835: Rename AST optimization related stuff after moving const folding to the peephole optimizier Mar 28, 2025
@WolframAlph WolframAlph marked this pull request as ready for review March 28, 2025 14:35
@WolframAlph WolframAlph requested a review from picnixz March 28, 2025 14:46
@iritkatriel
Copy link
Member

I'm not sure about the name ast_process. Seems to nonspecific. I don't have a better idea at the moment though.

@WolframAlph
Copy link
Contributor Author

WolframAlph commented Mar 28, 2025 •
edited
Loading

It does all kind of things. Optimizes format, removes docstrings, implements PEP 765, etc. I don't think there is good name for it to make it more specific.

@WolframAlph
Copy link
Contributor Author

@picnixz Maybe you have better suggestion how to name it?

@picnixz
Copy link
Member

picnixz commented Mar 28, 2025 •
edited
Loading

I think "ast_process" is fine but otherwise, something with AST_Transform as we're doing a transformation/checks on the AST. I feel there is a mix of AST optimization (real opts + docstrings eradication), a bit of AST processing/post-processing (PEP 765) and more. So process is fine IMO.

@iritkatriel
Copy link
Member

Maybe ast_preprocess. Preprocessing is a term you hear in compilers.

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

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

Yes, I think preprocess is better as it's before we compile (and compilers would call it a pre-processing step (e.g., C compilers would pre-process macros here and comments elimination; well optimization is not part of pre-processing and usually happens after but still before compilation I think?).

@iritkatriel iritkatriel mentioned this pull request Mar 31, 2025
Closed
@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@Eclips4
Copy link
Member

Eclips4 commented May 1, 2025

Hi @WolframAlph, are you still planning to work on this? We need to ship it soon since the feature freeze is approaching (May 6th).

Regarding the naming—I believe ast_preprocess is the most appropriate choice here.

@WolframAlph
Copy link
Contributor Author

Hi sorry for the delay, Ive been super busy last weeks. I will finish this ASAP.

@WolframAlph
Copy link
Contributor Author

@Eclips4 I've updated the PR. Please take a look.

Eclips4
Eclips4 approved these changes May 4, 2025
View reviewed changes
Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Eclips4 Eclips4 requested a review from picnixz May 4, 2025 12:35
@Eclips4 Eclips4 merged commit 0a1fedb into python:main May 4, 2025
42 checks passed
diegorusso added a commit to diegorusso/cpython that referenced this pull request May 4, 2025
@diegorusso
Merge remote-tracking branch 'origin/main' into call-len
c03babc 
* origin/main: (111 commits)
  pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385)
  pythongh-131178: Add tests for `ast` command-line interface (python#133329)
  Regenerate pcbuild.sln in Visual Studio 2022 (python#133394)
  pythongh-133042: disable HACL* HMAC on Emscripten (python#133064)
  pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387)
  pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946)
  pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388)
  pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830)
  pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368)
  pythongh-132457: make staticmethod and classmethod generic (python#132460)
  pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812)
  pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490)
  pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517)
  pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628)
  pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358)
  bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226)
  pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287)
  pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364)
  pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027)
  pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AA-Turner AA-Turner AA-Turner left review comments

@picnixz picnixz picnixz approved these changes

@Eclips4 Eclips4 Eclips4 approved these changes

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

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

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner

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

@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel 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
@WolframAlph @iritkatriel @picnixz @Eclips4 @AA-Turner