cberner / redb

An embedded key-value database in pure Rust
https://www.redb.org
Apache License 2.0
3.37k stars 156 forks source link

Turn on pedantic clippy lints where possible #886

Closed casey closed 2 weeks ago

casey commented 3 weeks ago

I like the pedantic clippy lints, since they often turn up things that are either a little suspicious, or which just have better alternatives.

I went through all the clippy::pedantic lints which weren't enabled, and one by one either denied or enabled them, usually with some kind of rationale. Sometimes I allowed lints that should probably be denied, either because they're subjective, or would take more knowledge of redb than I have to actually fix.

I tried to do each one in a separate commit, to keep things readable and let you review the rationale for each, which are in the commit messages.

Finally, I added clippy::pedantic to the #![deny] block, and tried to remove any allow which was not actually being triggered.

As of Rust 1.74, you can put lints in Cargo.toml, which is a nice way to enforce them project wide, but until then putting them at the top of lib.rs is reasonable.

Feel free to take or leave anything in this PR, since obviously a lot of it is subjective. And definitely squash this commit before merging!

casey commented 3 weeks ago

Just noticed the errors on CI on x86, will fix those and undraft.

casey commented 3 weeks ago

Fixed! I didn't realize that the xxh3 module was copied from another repo, so I reverted the changes and added #[allow(clippy::pedantic)] to the whole module.

cberner commented 3 weeks ago

Thanks! I'll look through these Sent from my phone

On Sun, Nov 3, 2024, 11:01 AM Casey Rodarmor @.***> wrote:

Fixed! I didn't realize that the xxh3 module was copied from another repo, so I reverted the changes and added #[allow(clippy::pedantic)] to the whole module.

— Reply to this email directly, view it on GitHub https://github.com/cberner/redb/pull/886#issuecomment-2453541812, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGNXQBWRZ4X2WS4TRMT2N3Z6ZQGZAVCNFSM6AAAAABRBDHUQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJTGU2DCOBRGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

cberner commented 2 weeks ago

These seem great, thanks!

casey commented 2 weeks ago

Nice! Consider taking a look at these and seeing if they seem worth enabling. In particular, Option<Option<T>> is a travesty 😂

clippy::if_not_else
clippy::inline_always
clippy::let_underscore_drop
clippy::must_use_candidate
clippy::option_option
clippy::redundant_closure_for_method_calls
clippy::unnecessary_wraps
clippy::unreadable_literal
cberner commented 2 weeks ago

Haha, ya that return type is a bit sketch =P