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

Use Result as a replacement for ControlFlow in Visitor #646

Closed

Conversation

LeSeulArtichaut
Copy link
Contributor

This PR (almost) resolves one of the differences between rustc's TypeVisitor and chalk's Visitor by adopting rustc's design with std::ops::ControlFlow.

Supersedes #645 with another approach suggested by @jackh726: instead of defining an enum ControlFlow, define a type alias type ControlFlow = Result, which allows to use ? instead of a custom macro.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Seems good to me

@bors
Copy link
Contributor

bors commented Nov 5, 2020

☔ The latest upstream changes (presumably #648) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@jackh726
Copy link
Member

jackh726 commented Nov 5, 2020

Gonna close this in favor of #645. Thanks anyways @LeSeulArtichaut for writing an alternative implementation here. Overall, it's a neat idea, but I think the simple approach is probably better.

@jackh726 jackh726 closed this Nov 5, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the visitor-result branch November 5, 2020 17:31
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.

4 participants