Skip to content

Improve CFG construction for match expressions #22075

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

Closed
wants to merge 3 commits into from

Conversation

Aatch
Copy link
Contributor

@Aatch Aatch commented Feb 8, 2015

Previously, for each arm, each pattern would be visited, then the guard, then the body. All arms would be visited in parallel.

Now, the patterns in each arm are visited in parallel, but after each pattern, the guard is visited. Furthermore, an edge is added from each guard to the next, reflecting the fact that a guard may be evaluated multiple times, and that multiple guards may be evaluated. This required changing dataflow to handle the fact that there is now a 1-many mapping of NodeIds -> CFGIndex.

This also:

  • Refactors the CFG a little to use explicit node types for entry, exit, dummy and unreachable nodes.
  • Extends the graphviz library to allow for customisation of the graph nodes. This is used for pretty-printing the CFG.
  • Shows the matches when requested item is not unique. Methods on generic types don't seem to be able to be matched, so this just prints out the items that match the suffix provided, which includes the node id.

Fixes #22073

Aatch added 3 commits February 8, 2015 17:41
Previously, for each arm, each pattern would be visited, then the guard,
then the body. All arms would be visited in parallel.

Now, the patterns in each arm are visited in parallel, but after each
pattern, the guard is visited. Furthermore, an edge is added from each
guard to the next, reflecting the fact that a guard may be evaluated
multiple times, and that multiple guards may be evaluated.

This also refactors the CFG a little to use explicit node types for
entry, exit, dummy and unreachable nodes.
This allows the implementor of the Labeller trait to provide a set of
attributes for the node.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@pnkfelix
Copy link
Member

can you make this return an Option

Thinking further on this point, I wonder if instead all of the fields in NodeAttributes should be Options. Because under my suggestion above, you get clean output if you attach no attributes, but you still will get a fair amount of boilerplate when all you want is to insert a single attribute.

@Aatch
Copy link
Contributor Author

Aatch commented Feb 13, 2015

@pnkfelix in light of the work I'm currently doing, I may just revert the graphviz-specific changes. I've written a much more full-featured system that 1) avoids outputting attributes that are the default, 2) allows for node and edge attributes to be set at the graph level and 3) allows both nodes and edges to be set.

@pnkfelix
Copy link
Member

@Aatch that sounds fine with me. So I'll just focus on reviewing the changes to the control-flow graph itself.

@pnkfelix
Copy link
Member

(I'll be posting a PR with a subset of the work here shortly. Thanks a ton @Aatch; I'm making sure to retain your authorship meta-data on the new commits.)

@pnkfelix
Copy link
Member

closing in favor of #22580 (which is essentially the same as the first commit in this PR, apart from some factoring into separate commits, cutting of some details I decided were not strictly necessary, and slight fixes to the very helpful comment from @Aatch ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moves in guard expressions aren't handled properly
3 participants