Skip to content
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

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Conversation

topecongiro
Copy link
Contributor

This PR applies refactoring suggested from running cargo clippy.

Copy link
Member

@nrc nrc left a 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 {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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>> {
Copy link
Member

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.

@topecongiro
Copy link
Contributor Author

Updated & resolved conflicts.

@nrc nrc merged commit d08405e into rust-lang:master Aug 31, 2017
@nrc
Copy link
Member

nrc commented Aug 31, 2017

Thank you!

@topecongiro topecongiro deleted the cargo-clippy branch September 15, 2017 14:25
andersk added a commit to andersk/rustfmt that referenced this pull request Sep 16, 2017
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants