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.

Sign up for GitHub

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

Jump to bottom

Use ControlFlow in Visitor #645

Merged
merged 2 commits into from
Nov 5, 2020
Merged

Use ControlFlow in Visitor #645

merged 2 commits into from
Nov 5, 2020

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.

However as std::ops::ControlFlow is still unstable, this PR defines a duplicate of ControlFlow in chalk_ir along with a try_break! macro which behaves like try!.

cynecx
cynecx reviewed Oct 31, 2020
View reviewed changes
chalk-ir/src/visit.rs
} else {
self.combine(op())
/// An copy of the unstable `std::ops::ControlFlow` for use in Chalk visitors.
pub enum ControlFlow<B, C = ()> {
Copy link

Choose a reason for hiding this comment

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

Perhaps this type should be #[must_use]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this was also suggested in rust-lang/rust#78182 (comment) so I had opened rust-lang/rust#78202, but that PR didn't gather much attention.

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Oct 31, 2020
Closed
@bors
Copy link
Collaborator

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

@bors r+

While #646 is "clever", and allows us to remove try_break in favor of ?, I don't think that really outweighs the slight weirdness. try_break is easily enough to grep for when ControlFlow is stabilized.

@bors
Copy link
Collaborator

bors commented Nov 5, 2020

📌 Commit c3d0a61 has been approved by jackh726

@bors
Copy link
Collaborator

bors commented Nov 5, 2020

⌛ Testing commit c3d0a61 with merge f2e3fd9...

@bors
Copy link
Collaborator

bors commented Nov 5, 2020

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing f2e3fd9 to master...

@bors bors merged commit f2e3fd9 into rust-lang:master Nov 5, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the visitor-controlflow branch November 5, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cynecx cynecx cynecx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LeSeulArtichaut @bors @jackh726 @cynecx