Make many MacResult methods unreachable. by nnethercote · Pull Request #142061 · 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
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 }})
- Every method in the
MacResulttrait can be unreachable exceptmake_stmts. - Lots of
MacEagermethods can removed, so they fall back to the default unreachable methods. Because of this, theimpl_items/trait_items/foreign_itemsfields are never used and can also be removed.
Every method in the
MacResulttrait can be unreachable exceptmake_stmts.Lots of
MacEagermethods can removed, so they fall back to the default unreachable methods. Because of this, theimpl_items/trait_items/foreign_itemsfields are never used and can also be removed.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
@petrochenkov: see what you think of this. I don't fully understand the details of the affected code. I was trying to work out how some of these methods are called, and so I inserted a bunch of unreachable!() calls and discovered that they aren't. Maybe they are genuinely unreachable, maybe there is some missing test coverage, I'm not sure. I think if a method truly is unreachable it's good to have unreachable!() in it, it makes the code more understandable.
| trait_items: SmallVec<[Past::AssocItem\; 1]>, |
|---|
| foreign_items: SmallVec<[Past::ForeignItem\; 1]>, |
| stmts: SmallVec<[ast::Stmt; 1]>, |
| ty: Past::Ty\, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MacEager can certainly be trimmed further, only MacEager::{expr, items, ty} are actually used in built-in macros, the other fields are never set to anything other than None and their uses can be replaced with None.
| fn make_variants(self: Box<DummyResult>) -> Option<SmallVec<[ast::Variant; 1]>> { |
| Some(SmallVec::new()) |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need
- a test using
#[rustfmt::skip]in all possible attribute positions, - a test using
log_syntax!()in all possible fn-like macro positions, - and a test using
include!()orconcat_idents!()in all possible fn-like macro positions.
If all of those do not crash, then we can keep the unreachable!s.
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.