Turn on pedantic clippy lints where possible #886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These drop calls seem not to be necessary.
Casts, i.e., the `as` syntax, should generally be avoided where possible. They truncate by default, which is dangerous if it isn't what you want. This lint complains about `as` when the cast is lossless, which means you can use `T::from` or `.into()`. I tried to use `.into()` where possible due to type inference, becuase it's slightly nicer, and avoids having to name the types involved.
This avoids an assert, because on the next line we do the conversion with an unwrap, which has the same effect.
Copied should generally be used where possible, because all types which implement copy implement clone, but not all types which implement copy, so using `.copied()` gives the additional information that the copy is cheap.
This one is just kind of annoying, forcing you to write `T::default()` instead of `Default::default()`, so just ignore it.
Clippy wants items in markdown documentation to be inside backticks. Why not, since it makes them formatted nicely.
`for x in foo.iter()` is equivalent to `for x in &foo`, which is shorter and more idiomatic
This complains if you do `if !x { … } else { … }`, on the basis that you
could avoid the `!` and just switch the bodies of the if and else. I
like this one, but it's subjective and creates a lot of code churn, so
I allowed it. If you like it, think about removing it from the allow
block.
Clippy doesn't like this and suggests always profiling first, but I have no idea if these instances are correct or not, so allow them.
Clippy complains if the return type from a function called `iter` does not implement `Iterator`. This is an API break or a new implementation of `Iterator`, so just allow it.
Clippy doesn't like if you use `let _ = x` when `x` implements `Drop`, since it's not necessarily clear that `_` will invoke `Drop`. (Unlike an `_`-prefixed identifier, which will not drop.) This is kind of subjective, so allow it.
Clippy prefers `map_or` to `map + unwrap_or`. Why not.
Clippy wants you to write docs about panics and errors. Probably a good idea, but who has time for that. Allow it.
Replace a `_` match for the single variant it matches. Seems reasonable.
Clippy doesn't like repitition of module names inside types. Who cares. Allow it.
Clippy complains if you pass a type by value when it isn't consumed, so it could be passed by reference. I think this should be evaluated on a case-by-case basis, depending on ergonomics, so allow it.
`Option<Option<T>>` is clearly an insane type, but I dare not touch it, because I have no idea what it's supposed to mean, so allow it.
Using `a..=b` is clearer and shorter than `a..(b + 1)`.
In these `where` clauses, `'b` outlives `'a`, so it's enough to declare that `Self: 'b`.
This is nice, since it reminds you where you can inline things in format strings.
This should probably be denied, but in some cases, a function returns `Result` or `Option` for API compatibility reasons, and I didn't want to wade into that.
You can omit a semicolon after a `()` expression, but seems fine to
deny it for consistent formatting. Add an exception for this lunacy:
fn from_bytes<'a>(_data: &'a [u8]) -> ()
where
Self: 'a,
{
()
}
You're not the boss of me.
This is just silly. I'll make similar names if I want to.
More nanny-state bullshit.
I do actually think that most of these could use some `_` separators, the decimal literals should be in groups of three, and the hex should be maybe every 4 bytes, or something like that, but this is subjective.
These seem reasonable to remove. These else blocks are after an if block which either returns or breaks the loop, so these get rid of some nesting.
It seems clearer to avoid taking `&self` if it isn't used, and these aren't public APIs, so it's backwards compatible.
This is a reasonable lint, but it's subjective and would be a larger change, so I was too lazy to do it.
Match arms with duplicate captures and bodies can be deduplicated. Seems reasonable to me.
This seems reasonable, small copy values should be passed by value.
I think this is actually a good one, but fixing it was annoying, so allow it.
Clippy doesn't like mem::transmute for transmuting pointers to pointers. I think the logic is that `mem::transmute` will transmute absolutely any type into any other type, as long as they're the same size. A pointer cast is much more limited, so it seems reasonable to use it instead.
|
Just noticed the errors on CI on x86, will fix those and undraft. |
|
Fixed! I didn't realize that the xxh3 module was copied from another repo, so I reverted the changes and added |
|
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
<#886 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGNXQBWRZ4X2WS4TRMT2N3Z6ZQGZAVCNFSM6AAAAABRBDHUQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJTGU2DCOBRGI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
These seem great, thanks! |
|
Nice! Consider taking a look at these and seeing if they seem worth enabling. In particular, |
|
Haha, ya that return type is a bit sketch =P |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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::pedanticlints 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 ofredbthan 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::pedanticto the#![deny]block, and tried to remove anyallowwhich 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 oflib.rsis 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!