-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add more functions for ensuring nodes via MastForestBuilder #1404
Add more functions for ensuring nodes via MastForestBuilder #1404
Conversation
Hey @plafer reviewers aren't assigned automatically and I can't request reviews. This one is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you for tackling this one as well!
Overall, I think the solution works but we probably want to add convenience methods for other nodes too (e.g., for Join
, and Split
nodes). For these, I don't think we can use the Into
approach (or at least I'm not sure how). So, we may end up needed MastForestBuilder::ensure_join()
, MastForestBuilder::ensure_split()
etc.
If we do the above, we might want to have a similar approach for basic blocks. Though, I agree, MastForestBuilder::ensure_basic_block()
does not look as elegant as the current solution. Maybe, we can just say MastForestBuilder::ensure_block()
.
Separately, and not for this PR, I find the fact that we need to say "basic_block" in various places pretty cumbersome. Maybe we should just say "block" everywhere instead. @plafer and @bitwalker, curious what you think on this.
Here is an example where instead of: let nested = {
let node = MastNode::new_split(r#true2, r#false2, expected_mast_forest_builder.forest());
expected_mast_forest_builder.ensure_node(node)
}; We should be able to do something like: let nested = expected_mast_forest_builder.ensure_split(r#true2, r#false2); |
c53df25
to
df767d8
Compare
@bobbinth I have made the suggested changes. LMK if you prefer having separate functions as opposed to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left one small comment inline. After it is addressed, we can merge.
LMK if you prefer having separate functions as opposed to the
Option<DecoratorList>
forensure_block()
.
I think the way you have it now looks good - so, no need to change anything in this regard.
Just wondering whether we want to do the same for let dyn_node_id = {
let node = MastNode::new_dyn();
mast_forest.add_node(node).unwrap()
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
Just wondering whether we want to do the same for
MastForest::add_node()
Yes - that would be great! But I'd do it in a separate PR.
Closes #1355
Add methods to
MastForestBuilder
to facilitate ensuring nodes with fewer LOC