ast/simplify: Remove in_lvalue/in_param simplify() parameters #3813
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(It doesn't make much sense to separate these three commits, so it's one PR.) |
povik
force-pushed
the
ast-simplify-work-vol2
branch
from
c9bafd9 to
d6284f1
Compare
|
Rebased, shaked off the commits that were merged in PR #3722 |
zachjs
approved these changes
Sep 19, 2023
povik
force-pushed
the
ast-simplify-work-vol2
branch
from
d6284f1 to
75225e1
Compare
|
Rebased, but as it is now those old way/new way equivalence asserts between the second and third commit fail at some point in the test suite. Maybe I will investigate. (Those asserts are removed in the final commit so it's an option to merge this as is.) |
The following commit will replace the way in_lvalue/in_param is being tracked in the simplify code. Make tweaks in advance so that it will be easier to make the old way and the new way agree. These changes all should be innocuous.
Instead of passing around in_lvalue/in_param flags to simplify, we make the flags into properties of the AST nodes themselves. After the tree is first parsed, we once do ast->fixup_hierarchy_flags(true) to walk the full hierarchy and set the flags to their initial correct values. Then as long as one is using ->clone(), ->cloneInto() and the AstNode constructor (with children passed to it) to modify the tree, the flags will be kept in sync automatically. On the other hand if we are modifying the children list of an existing node, we may need to call node->fixup_hierarchy_flags() to do a localized fixup. That fixup will update the flags on the node's children, and will propagate the change down the tree if necessary. clone() doesn't always retain the flags of the subtree being cloned. It will produce a tree with a consistent setting of the flags, but the root doesn't have in_param/in_lvalue set unless it's intrinsic to the type of node being cloned (e.g. AST_PARAMETER). cloneInto() will make sure the cloned subtree has the flags consistent with the new placement in a hierarchy. Add asserts to make sure the old and new way of determining the flags agree.
povik
force-pushed
the
ast-simplify-work-vol2
branch
from
75225e1 to
a511976
Compare
Should be resolved now (as expected, it was due to interaction with other AST changes in the meantime). |
zachjs
approved these changes
Sep 28, 2023
|
If that's the case, let's do it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split off from PR #3722,
but it's still on top of the other PR's commits to save on rebase work. So, only the last three commits, starting with "ast/simplify: Make tweaks in advance of big in_lvalue/in_param change" should be the subject of this PR.As is, the change might be objectionable since it requires the code to be sprinkled with calls to fixup_hierarchy_flags and set_attribute (new methods). That may not be as bad if we (1) don't require the in_param/in_lvalue values of the old code to be perfectly reproduced (which the change here does reproduce, at least as exercised by make test); (2) move to a coding style in the simplify code that doesn't rely on explicitly modifying the children list of an existing node as much. See the commits and let me know what you think.
This should be passing make test at each commit.