-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Implement RFC 2539: cfg_attr with multiple attributes #54862
Conversation
Does not implement the warning or a feature flag.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
parser.eat(&token::Comma); // Optional trailing comma | ||
|
||
// Presumably, the majority of the time there will only be one attr. | ||
let mut expanded_attrs = Vec::with_capacity(1); |
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.
Maybe premature, but have you considered using SmallVec to save an allocation?
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.
I don't know how to measure whether or not that would be a win. And yes, it's a bit premature, since I want to get the code right first.
Sorry for the delay, will review tomorrow. |
// compile-pass | ||
|
||
#![cfg_attr(all(),)] | ||
#![cfg_attr(all(), feature(cfg_attr_multi), crate_type="bin")] |
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.
This is a fun example.
I didn't know that crate attributes are configured twice to get the correct feature list early.
@bors r+ |
📌 Commit bbe832d has been approved by |
Rollup of 9 pull requests Successful merges: - #54747 (codegen_llvm: verify that inline assembly operands are scalars) - #54848 (Better Diagnostic for Trait Object Capture) - #54850 (Fix #54707 - parse_trait_item_ now handles interpolated blocks as function body decls) - #54858 (second round of refactorings for universes) - #54862 (Implement RFC 2539: cfg_attr with multiple attributes) - #54869 (Fix mobile docs) - #54870 (Stabilize tool lints) - #54893 (Fix internal compiler error on malformed match arm pattern.) - #54904 (Stabilize the `Option::replace` method) Failed merges: - #54909 ( Add chalk rules related to associated type defs) r? @ghost
Rollup of 9 pull requests Successful merges: - #54747 (codegen_llvm: verify that inline assembly operands are scalars) - #54848 (Better Diagnostic for Trait Object Capture) - #54850 (Fix #54707 - parse_trait_item_ now handles interpolated blocks as function body decls) - #54858 (second round of refactorings for universes) - #54862 (Implement RFC 2539: cfg_attr with multiple attributes) - #54869 (Fix mobile docs) - #54870 (Stabilize tool lints) - #54893 (Fix internal compiler error on malformed match arm pattern.) - #54904 (Stabilize the `Option::replace` method) Failed merges: - #54909 ( Add chalk rules related to associated type defs) r? @ghost
This PR implements RFC 2539, except for the lint.
The lint is non-trivial and will be done in its own PR.
The tracking issue
555666
used in this PR does not actually exist, since there's no tracking issue for this RFC at the time of this. Once there's a tracking issue, I'll update the URL.I moved
src/ui/cfg-attr-trailing-comma.rs
intosrc/ui/conditional-compilation/parse.rs
instead of having it be its own test file.r? @petrochenkov
There is one problem with getting the error message to say
expected identifier or `)`
forcfg_attr(pred, attr,,))
. Instead, it just saysexpected identifier
. It also says that when the feature gate is not enabled, which is definitely wrong and confusing. The expected identifier message comes fromParser::expected_ident_found
which doesn't look at the `Parser::expected_tokens vec. I'm wondering if I should fix that in this PR as well.