Use ops::ControlFlow in rustc_data_structures::graph::iterate by scottmcm · Pull Request #76318 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation6 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Since I only know about this because you mentioned it,
r? @ecstatic-morse
If we're not supposed to use new core things in compiler for a while then feel free to close, but it felt reasonable to merge the two types since they're the same, and it might be convenient for people to use ? in their traversal code.
(This doesn't do the type parameter swap; NoraCodes has signed up to do that one.)
| /// It's frequently the case that there's no value needed with `Continue`, |
|---|
| /// so this provides a way to avoid typing `(())`, if you prefer it. |
| #[unstable(feature = "control_flow_enum", reason = "new API", issue = "75744")] |
| pub const CONTINUE: Self = ControlFlow::Continue(()); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments welcome on whether this is a good idea.
I could also imagine other things like
enum ControlFlow<B, C=()> { BreakWith(B), ContinueWith(C) }
impl<B> ControlFlow<B> { pub const Continue: Self = ControlFlow::ContinueWith(()); }
But that doesn't seem like how things are done elsewhere...
(And of course there's always the answer of just not having a constant and typing the (()).)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All are intriguing. Also possible would be an associated constant with the exact same name as the variant, since type inference should do a good job of choosing between the enum variant constructor (fn(()) -> ControlFlow) and the constant (ControlFlow) in most cases.
I tend to think we should avoid "magic" (a constant that looks like a variant), so I think I prefer either the upper-case associated constant or writing out the (()). If you like the associated constant, there should probably be one for Break as well.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also possible would be an associated constant with the exact same name as the variant
I tried this out for fun, and it's weird. I don't understand why it's allowed (since I thought constants and variants were both in value namespace), but the constant seems to always be hidden: #76347
I'll add the BREAK equivalent to this PR and ping libs as requested.
LGTM. We should mention the associated constants in the unresolved questions on #75744 and ping T-libs somewhere, since I think we should start trying to build consensus as early as possible. r=me once that is done.
📌 Commit 59e3733 has been approved by ecstatic-morse
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request
…ic-morse
Use ops::ControlFlow in rustc_data_structures::graph::iterate
Since I only know about this because you mentioned it, r? @ecstatic-morse
If we're not supposed to use new core things in compiler for a while then feel free to close, but it felt reasonable to merge the two types since they're the same, and it might be convenient for people to use ? in their traversal code.
(This doesn't do the type parameter swap; NoraCodes has signed up to do that one.)
This was referenced
Sep 6, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.