gh-126835: Move constant unaryop & binop folding to CFG #129550
Conversation
WolframAlph
requested review from
isidentical,
Eclips4,
markshannon and
iritkatriel
as code owners
WolframAlph
force-pushed
the
fold-binop-cfg
branch
from
7140ad4 to
a6babab
Compare
WolframAlph
changed the title
Lib/test/test_ast/test_ast.py
Outdated
| value=ast.Constant(value=1), | ||
| ) | ||
| ) | ||
| self.assert_ast(result_code, non_optimized_target, optimized_target) | ||
|
|
||
| def test_folding_match_case_allowed_expressions(self): | ||
| source = textwrap.dedent(""" |
There was a problem hiding this comment.
Do we also want to check for invalid expressions like duplicate mapping keys? For example:
match x:
case {0j: y, 0j: z} ...There was a problem hiding this comment.
They won't trigger error here, this is only ast parsing phase. Error is emitted in codegen.
There was a problem hiding this comment.
yeah but it's not tested anywhere currently and since we're changing the logic around match, it feels safer to make sure we don't change something accidentally. Though I don't insist on adding it in this PR, it's more of a general comment
There was a problem hiding this comment.
We should test that, but I am not sure if in this PR as it only touches AST related logic.
There was a problem hiding this comment.
You can add tests covering various aspects of the feature you're touching.
|
I've addressed comments & updated ast_opt.c to not use macros for node attribute access. |
|
@iritkatriel CI reports unused functions, but we need them in assertions. So either mark them as used or convert them to macros I guess? |
Put them under |
…nary_expr only on debug builds
|
I've addressed review & simplifed match pattern folding. One question remaining whether we should add tests with hand crafted ast with invalid expressions in match statement case. |
|
This is looking good. I made some final minor comments. Are you still planning to add tests for invalid hand-crafted ASTs? |
|
Done. Honestly I don't think we need them. We already test folding in match cases quite extensively, and current folding is the same as previous, just for limited expressions. We could add them for completeness, but probably in another PR as this one is getting quite big. But if you think we need them, I could add to this. |
|
Sure. |
iritkatriel
added
the
🔨 test-with-refleak-buildbots
|
It would be good to add smoke tests to make sure we don't crash on invalid ast input. Can be in another pr. |
|
Thanks @iritkatriel for guidance & patience. |
iritkatriel
added
🔨 test-with-refleak-buildbots
|
!buildbot refleak |
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit cafbc61 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F129550%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
This PR migrates:
cc @Eclips4 @tomasr8