-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Give better error for macro_rules name
#89257
Conversation
/// The first bool is whether it's a macro_rules item. | ||
/// The second is whether the `!` after `macro_rules` is present | ||
fn is_macro_rules_item(&mut self) -> (bool, bool) { |
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.
nit: Seems like this code might be clearer if the return type were a struct with two boolean fields? E.g.:
struct IsMacroRulesItem {
is_macro_rules: bool,
has_bang_after_macro_rules: bool,
}
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.
Or return Option<IsMacroRulesItem>
with struct IsMacroRulesItem { has_bang: bool }
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.
Went with
enum IsMacroRulesItem {
Yes { has_bang: bool },
No,
}
we could bikeshed this forever, but I think this is pritty clear
/// The second is whether the `!` after `macro_rules` is present | ||
fn is_macro_rules_item(&mut self) -> (bool, bool) { | ||
if self.check_keyword(kw::MacroRules) { | ||
let macro_rules_span = self.look_ahead(0, |t| t.span); |
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.
Isn't this equivalent to self.token.span
?
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.
Yep, didn't realise you could do that
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.
After addressing the nitpicks, r=me
1ffff6f
to
729ff2d
Compare
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
@bors r=estebank |
📌 Commit 729ff2d has been approved by |
…bank Give better error for `macro_rules name` follow up to rust-lang#89221 r? `@estebank` `@rustbot` modify labels: +A-diagnostics +A-parser
Rollup of 14 pull requests Successful merges: - rust-lang#87537 (Clarify undefined behaviour in binary heap, btree and hashset docs) - rust-lang#88624 (Stabilize feature `saturating_div` for rust 1.58.0) - rust-lang#89257 (Give better error for `macro_rules name`) - rust-lang#89665 (Ensure that pushing empty path works as before on verbatim paths) - rust-lang#89895 (Don't mark for loop iter expression as desugared) - rust-lang#89922 (Update E0637 description to mention `&` w/o an explicit lifetime name) - rust-lang#89944 (Change `Duration::[try_]from_secs_{f32, f64}` underflow error) - rust-lang#89991 (rustc_ast: Turn `MutVisitor::token_visiting_enabled` into a constant) - rust-lang#90028 (Reject closures in patterns) - rust-lang#90069 (Fix const qualification when executed after promotion) - rust-lang#90078 (Add a regression test for issue-83479) - rust-lang#90114 (Add some tests for const_generics_defaults) - rust-lang#90115 (Add test for issue rust-lang#78561) - rust-lang#90129 (triagebot: Treat `I-*nominated` like `I-nominated`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
follow up to #89221
r? @estebank
@rustbot modify labels: +A-diagnostics +A-parser