-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Merged by Bors] - Nightly clippy fixes #2702
Conversation
- Unnecessary boxing, collapsing if, adding unwrap_or_default
Nested case returns a separate value
@@ -136,6 +136,7 @@ impl RenderResourceBindings { | |||
} | |||
} | |||
|
|||
#[allow(clippy::collapsible_match)] |
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.
Is this a bug in the new clippy rule? With the else in line 152, the change clippy suggests would change the logic.
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.
The clippy docs state "this lint is not intended to find all cases where nested match patterns can be merged" (https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match). I interpreted this to mean that in cases where the nested match makes sense - like this one where the nested if returns a different value - you can just ignore it.
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.
As @DJMcNab just pointed out, the other change for this rule also breaks the logic. The rule seems to suggest this change too easily imo.
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 breaks ScheduleRunnerSettings::default()
and possibly also breaks if any frame is longer than the set time period.
if let Some(delay) = delay { | ||
std::thread::sleep(delay); | ||
} | ||
while let Ok(Some(delay)) = tick(&mut app, wait) { |
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 changes the semantics to be wrong, specifically when the runner settings are to run as often as possible
I'm shocked that clippy is suggesting this change to be honest.
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.
Should I change it back to what it was and just add an #allow for this lint then?
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.
Yeah, I would just revert and #allow.
This issue is already reported to clippy: rust-lang/rust-clippy#7575
if some of the lints on nightly have issues, I think they should not be fixed (or allowed) on Bevy until the lints are stable |
bors r+ |
A few minor changes to fix warnings emitted from clippy on the nightly toolchain, including redundant_allocation, unwrap_or_else_default, and collapsible_match, fixes #2698
A few minor changes to fix warnings emitted from clippy on the nightly toolchain, including redundant_allocation, unwrap_or_else_default, and collapsible_match, fixes #2698