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

Enable some lints and simplify CLIPPY=1 #566

Merged
merged 8 commits into from
Nov 26, 2021
Merged

Conversation

ojeda
Copy link
Member

@ojeda ojeda commented Nov 26, 2021

This simplifies a bit the Makefile and enables some extra lints:

-Dunreachable_pub
-Dnon_ascii_idents

-Drustdoc::missing_crate_level_docs

-Dclippy::suspicious
-Dclippy::let_unit_value
-Dclippy::mut_mut
-Dclippy::needless_bitwise_bool
-Dclippy::needless_continue
-Wclippy::dbg_macro

If some of these end up being too painful, we can always disable them later. But in general they sound like a good idea as long as they are not too annoying (i.e. an allow here or there). So far, most code "just worked". The only cases I had to change were:

  • unreachable_pub has found a few improvements already, and while its docs say it is pending some fixes on top, it seems good enough.
  • let_unit_value requires a small change for the unit guard, but I think it makes the code a bit more clear anyway.

I am also contemplating always using the Clippy driver and let kernel developers get accustomed to it -- i.e. no CLIPPY=1 anymore. This has the downside that it has a slight but noticeable performance penalty even if everything is allow (I guess it is going through the AST on its own, i.e. independently of rustc, which I guess could be improved if it was integrated with rustc instead). But it is not that big, so if Clippy upstream confirms there is no other downside apart from performance (rust-lang/rust-clippy#8035), then I think it may be the best path forward. In such a case, we could also consider moving some of them to W= instead, and possibly enabling others in other groups too.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
    -Dnon_ascii_idents
    -Drustdoc::missing_crate_level_docs
    -Dclippy::suspicious
    -Dclippy::mut_mut
    -Dclippy::needless_bitwise_bool
    -Dclippy::needless_continue
    -Wclippy::dbg_macro

`dbg_macro` is left as a warning since developers may be using `CLIPPY=1`
for normal development.

We may want to change how the entire `CLIPPY=1` works, possibly moving
to `W=` anyway and always enabling it.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Each tool including `rustc` itself, ignores unknown flags if
in a namespace like `rustdoc::` or `clippy::`, thus we can
keep all of them in a single list.

Furthermore, we already skip Clippy for `core` and `alloc`.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda
Copy link
Member Author

ojeda commented Nov 26, 2021

Thanks for the super quick review!

@ojeda ojeda merged commit 795c733 into Rust-for-Linux:rust Nov 26, 2021
@ojeda ojeda deleted the lints branch November 26, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants