Conversation
cfallin
left a comment
There was a problem hiding this comment.
Thanks!
FWIW I've avoided Clippy in the past because many of its lints and "fixes" are over-aggressive and opinionated, and sometimes make code worse (and likewise we feel that way in Wasmtime)... but most of these are actually pretty reasonable. On the other hand I'm not a huge fan of the load-bearing && in the workqueue-insertion bits (if changed is logically separate from the if set.insert, push on queue action).
I guess there's a question of whether we do this once-off or add a lint in CI. Obviously we don't want to have to keep doing a manual chore. I still don't like the idea of "default Clippy" being a gatekeeper, but maybe we could enable some lints?
Here's our Wasmtime config in the root Cargo.toml, where we start with no lints and turn some reasonable ones on, then we run it in CI. Maybe we could add that in this PR as well to make it sticky? And keep the diff to what that stripped-down config from Wasmtime produces?
|
Understood! I followed your suggestions and copied the wasmtime lints and adjusted the diff to only fix those. The reason I added this is that I usually run clippy before pushing PR and my changes were drowned in other warnings so it was hard to tell if my changes were reasons for warnings or they were preexisting. But also feel free to just close this PR without merging 🙂 |
cfallin
left a comment
There was a problem hiding this comment.
Looks great, thanks! I still speak Rust with a 2015-edition accent so it's good to have tooling to modernize various bits :-)
Fix clippy warnings, most of the fixes were applied using
clippy --fix.