Skip to content

Result from running on a few crates #553

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

Closed
mcarton opened this issue Jan 14, 2016 · 4 comments
Closed

Result from running on a few crates #553

mcarton opened this issue Jan 14, 2016 · 4 comments
Labels
C-question Category: Questions S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@mcarton
Copy link
Member

mcarton commented Jan 14, 2016

PR on racer: racer-rust/racer#472
PR on rustful: Ogeon/rustful#91
PR on rust-bindgen: https://github.com/crabtw/rust-bindgen/pull/252
PR on quickcheck: BurntSushi/quickcheck#118
PR on Cargo: rust-lang/cargo#2282

Results

On cargo

warnings before:
     cmp_owned : 2
     cyclomatic_complexity : 1
     deprecated : 11
     explicit_iter_loop : 57
     identity_op : 3
     len_without_is_empty : 3
     len_zero : 20
     let_and_return : 3
     map_clone : 14
     needless_lifetimes : 4
     needless_return : 24
     ok_expect : 2
     option_map_unwrap_or : 10
     private_in_public : 28
     redundant_closure : 2
     should_implement_trait : 2
     single_match : 3
     str_to_string : 112
     string_to_string : 12
     type_complexity : 3
     unnecessary_mut_passed : 2
     unneeded_field_pattern : 3
     unused_lifetimes : 1
     unused_variables : 2
     while_let_loop : 2

warnings after:
     cmp_owned : 2
     cyclomatic_complexity : 1
     identity_op : 1
     let_and_return : 3
     map_clone : 1
     needless_return : 3
     ok_expect : 2
     option_map_unwrap_or : 1
     private_in_public : 28
     should_implement_trait : 2
     str_to_string : 80
     type_complexity : 3
     while_let_loop : 2
  • needless_return needs a macro check;
  • let_and_return should ignore:
    let foo : type = …;
    type
  • while_let_loop should ignore cases where the iterator is next()-ed somewhere else in the loop;
  • this:
    ret.to_doc_test = to_builds.iter().map(|&p| p.clone()).collect();

could not use cloned() as suggested by map_clone;

  • ok_expect was not on Rust 1.2, which is still supported by Cargo.

On racer

warnings before: https://gist.github.com/mcarton/3496037f45d315bedc9a#file-before
     cyclomatic_complexity : 4
     collapsible_if : 1
     toplevel_ref_arg : 9
     used_underscore_binding : 2
     wrong_self_convention : 1
     explicit_counter_loop : 5
     ptr_arg : 1
     type_complexity : 1
     str_to_string : 9
     match_ref_pats : 3

warnings after: https://gist.github.com/mcarton/3496037f45d315bedc9a#file-after
     wrong_self_convention : 1
     cyclomatic_complexity : 4
     explicit_counter_loop : 1
     type_complexity : 1
     str_to_string : 4

str_to_string needs a macro check. (#566)
I did not fix the explicit_counter_loop because of types (it'd need a cast u32 ↔ usize cast IIRC). This needs discussion.

On rustful

warnings before: https://gist.github.com/mcarton/6411c8526dee7b12632b#file-before
     match_ref_pats : 6
     cyclomatic_complexity : 13
     option_map_unwrap_or_else : 1
     type_complexity : 3
     while_let_on_iterator : 1
     ptr_arg : 1
     needless_lifetimes : 9
     redundant_closure : 7

warnings after: https://gist.github.com/mcarton/6411c8526dee7b12632b#file-after
     type_complexity : 3
     cyclomatic_complexity : 13
     needless_lifetimes : 7

I think most if not all of the needless_lifetimes are false positive (#599) and I don't think RouteIter<Split<'a, u8, fn(&u8) -> bool>> is that bad a type.

On rust-bindgen

warnings before:
     len_zero : 1
     used_underscore_binding : 3
     ok_expect : 1
     unknown_lints : 1
     unused_lifetimes : 2
     explicit_iter_loop : 2
     toplevel_ref_arg : 3
     string_to_string : 1
     needless_return : 51
     match_bool : 2
     collapsible_if : 1
     needless_lifetimes : 2
     unused_imports : 1
     ptr_arg : 5
     str_to_string : 41
     match_ref_pats : 9
     needless_range_loop : 1

warnings after:
     needless_lifetimes : 1

Again, I think the remaining needless_lifetimes is a false positive.

On quickcheck

warnings before:
     needless_return : 4
     len_zero : 1
     match_ref_pats : 1
     let_and_return : 1
     needless_range_loop : 1
     linkedlist : 4
     match_bool : 1

warnings after:
     linkedlist : 4

I don't think linkedlist should lint when implementing a public trait on LinkedList<_>: The trait is only implemented in case users would want to use LinkedList<_>, that's not a use of LinkedList<_> in the library itself.

Cyclomatic complexity lint

The cyclomatic complexity warns about:

in Cargo:
     39 : 1 fn
in racer:
     27 : 1 fn
     29 : 1 fn
     30 : 1 fn
     32 : 1 fn
in rustful:
     31 : 2 fn
     41 : 1 fn
     51 : 8 fn
     61 : 2 fn
in rust-bindgen and quickcheck:
     None :)

In rustful all of them are unfair because caused by calls to a relatively sane macro.

@mcarton mcarton changed the title Result from running on racer and rustful Result from running on racer, rustful and rust-bindgen Jan 15, 2016
@mcarton mcarton changed the title Result from running on racer, rustful and rust-bindgen Result from running on a few crates Jan 15, 2016
@mcarton
Copy link
Member Author

mcarton commented Jan 15, 2016

Just added Cargo.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 15, 2016

The CC-lint should be ignoring macros. I'll have to investigate that.

@Manishearth
Copy link
Member

Thanks for doing this! Most of these seem easy to fix, I'll have a go at fixing them Monday if someone doesn't do it first.

@phansch
Copy link
Member

phansch commented Dec 3, 2018

Going to close this as this issue is almost 3 years old and Clippy has come a long way since then ⌛

@phansch phansch closed this as completed Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Category: Questions S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

4 participants