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.

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

Fix modifyAllMatching visitor helper #5020

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Nov 19, 2024

A very simple fix of P4::modifyAllMatching visitor helper. Basically while the Modifier visitor guarantees that the result type of node transformation is the same as the original, the apply function type cannot reflect that as it has to be able to work with any visitor. Therefore, modifyAllMatching needs an explicit cast, otherwise it would work only on RootType = IR::Node.

@vlstill vlstill added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Nov 19, 2024
@vlstill vlstill self-assigned this Nov 19, 2024
@@ -820,7 +820,7 @@ const RootType *modifyAllMatching(const RootType *root, Func &&function) {
Func function;
void postorder(NodeType *node) override { function(node); }
};
return root->apply(NodeVisitor(std::forward<Func>(function)));
return root->apply(NodeVisitor(std::forward<Func>(function)))->template checkedTo<RootType>();
Copy link
Contributor Author

@vlstill vlstill Nov 19, 2024

Choose a reason for hiding this comment

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

template keyword needed as the RootType is a type parameter

@vlstill vlstill force-pushed the vstill/modify-all-matching branch from 56ff647 to b671a45 Compare November 19, 2024 09:47
@vlstill vlstill requested review from ChrisDodd and fruffy November 19, 2024 09:47
Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

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

I guess the point it to allow this to be called on nodes that are not expected to be root nodes and don't define an apply overload with covariant return type (don't have #apply in the .def file)? Seems like a rare corner case.

@vlstill
Copy link
Contributor Author

vlstill commented Nov 19, 2024

I guess the point it to allow this to be called on nodes that are not expected to be root nodes and don't define an apply overload with covariant return type (don't have #apply in the .def file)? Seems like a rare corner case.

Yes. For example a IR::BlockStatement. I agree it is niche, but it is still useful.

@vlstill vlstill added this pull request to the merge queue Nov 19, 2024
Merged via the queue into main with commit 9bef7ee Nov 19, 2024
19 checks passed
@vlstill vlstill deleted the vstill/modify-all-matching branch November 19, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants