-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(new_without_default): if new
has #[cfg]
, copy that onto impl Default
#15720
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
Conversation
rustbot has assigned @samueltardieu. Use |
This comment has been minimized.
This comment has been minimized.
6a4abd0
to
ae2ebd6
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This range-diff thing is awesome:) |
return; | ||
} | ||
|
||
let mut appl = Applicability::MachineApplicable; |
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.
Applicability is usually stored into app
or applicability
. There is only one existing instance of appl
(against more than 80 for the others), let's not use this.
[] => String::new(), | ||
[attr] if attr.has_name(sym::cfg_trace) => { | ||
format!( | ||
"{}\n", | ||
snippet_with_applicability(cx.sess(), attr.span(), "_", &mut appl) | ||
) | ||
}, | ||
_ => { | ||
// There might be some other attributes which the `impl Default` ought to inherit from `fn new`. | ||
// At the same time, there are many attributes that actually can't be put on an impl block -- | ||
// like `#[inline]`, for example. And for some attributes we can't even build a suggestion, | ||
// since `Attribute::span` may panic. Because of all this, remain conservative: don't inherit | ||
// any attrs, and just reduce the applicability | ||
appl = Applicability::MaybeIncorrect; | ||
String::new() | ||
}, | ||
}; |
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.
Why not extract all #[cfg]
attributes and repeat them? Why just one, and only if it is alone?
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 thought having multiple #[cfg]
attributes would be disallowed -- if one of them is false
, then the item under it is removed, leaving the rest of the attributes dangle -- but it turns out Rust is (unsurprisingly) smarter than that
ae2ebd6
to
a1ac9e6
Compare
@rustbot ready |
a1ac9e6
to
7a04ada
Compare
Fixes #14552
changelog: [
new_without_default
]: ifnew
has#[cfg]
, copy that ontoimpl Default