-
Notifications
You must be signed in to change notification settings - Fork 912
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
Apply refactoring from cargo clippy #1931
Conversation
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.
Mostly looks good, just a few changes I'd rather not make.
if let &WorkspaceHitlist::Some(ref hitlist) = self { | ||
Some(&hitlist) | ||
pub fn get_some(&self) -> Option<&[String]> { | ||
if let WorkspaceHitlist::Some(ref hitlist) = *self { |
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'm not sure why Clippy suggests this change - it looks fine either way to me
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 think the idea here is to prefer not to repeat &
in pattern:
match *self {
A => ...,
B => ...,
C => ...,
}
instead of
match self {
&A => ...,
&B => ...,
&C => ...,
}
I agree that it is irrelevant in the case of if let Some(..) = *self
v.s. if let &Some(..) = self
.
Just to keep it consistent, I would like to use the former style here as well, if you don't mind.
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.
Sure, I don't mind either way
src/expr.rs
Outdated
@@ -932,7 +932,7 @@ struct ControlFlow<'a> { | |||
span: Span, | |||
} | |||
|
|||
fn to_control_flow<'a>(expr: &'a ast::Expr, expr_type: ExprType) -> Option<ControlFlow<'a>> { |
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 think we should do this (or the similar changes elsewhere) - having the explicit lifetime inidicates that ControlFlow contains borrowed data.
9ee612c
to
9d49bd2
Compare
Updated & resolved conflicts. |
Thank you! |
This test was mistakenly inverted by commit 4b79055 (rust-lang#1931). $ cargo fmt --all thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:335:20 note: Run with `RUST_BACKTRACE=1` for a backtrace. Signed-off-by: Anders Kaseorg <[email protected]>
This PR applies refactoring suggested from running
cargo clippy
.