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

Review and address clippy exceptions. #13

Closed
eddyb opened this issue Dec 14, 2022 · 0 comments · Fixed by #14
Closed

Review and address clippy exceptions. #13

eddyb opened this issue Dec 14, 2022 · 0 comments · Fixed by #14
Milestone

Comments

@eddyb
Copy link
Contributor

eddyb commented Dec 14, 2022

In order to get CI to pass I originally added these clippy exceptions:

spirt/src/lib.rs

Lines 90 to 117 in b586de4

// crate-specific exceptions:
#![allow(
// FIXME(eddyb) review all of these (some of them are intentional, sadly).
clippy::collapsible_if,
clippy::comparison_to_empty,
clippy::get_first,
clippy::iter_nth_zero,
clippy::map_err_ignore,
clippy::match_same_arms,
clippy::match_wildcard_for_single_variants,
clippy::needless_borrow,
clippy::needless_lifetimes,
clippy::nonminimal_bool,
clippy::redundant_closure,
clippy::redundant_closure_call,
clippy::redundant_field_names,
clippy::semicolon_if_nothing_returned,
clippy::should_implement_trait,
clippy::single_match,
clippy::single_match_else,
clippy::string_add,
clippy::unimplemented,
clippy::unnested_or_patterns,
clippy::unused_self,
clippy::useless_conversion,
clippy::vtable_address_comparisons,
elided_lifetimes_in_paths,
)]

Most of them should be addressable, though there might strangely opinionated ones, or clippy bugs, that may require #[allow(...)]-ing some lints still.

@eddyb eddyb added this to the 0.1.0 milestone Dec 14, 2022
@eddyb eddyb closed this as completed in #14 Dec 14, 2022
eddyb added a commit that referenced this issue Dec 14, 2022
In no particular order, these are the suspicious and/or
not-entirely-addressed lints:
* `clippy::single_match_else`: permanently `#![allow(…)]`ed, as the
style it denies is important for readability
* `clippy::string_add`: permanently `#![allow(…)]`ed, as it's (IMO)
misguided (and perhaps buggy?)
* `(...: String) + "..."` is short for `{ let mut s = ...;
s.push_str("..."); s }`, *not something else*
* `clippy::match_same_arms`: `#[allow(…)]` moved to specific `match`es
where listing out the patterns individually is more important than what
the actual values are (i.e. grouping by the output values is
detrimental)
* `clippy::should_implement_trait`: `#![allow(…)]` moved to specific
module encountering this bug:
  * rust-lang/rust-clippy#5004
* `clippy::unused_self`: `#[allow(…)]` moved to specific `impl` (where
`self` may be used in the future)
* `clippy::redundant_closure_call`: `#[allow(…)]` moved to specific
macro using a closure as a `try {...}` block
* `clippy::map_err_ignore`: had to be worked around because the
`map_err(|_|` it complains about have no information in the error, i.e.
those `Result`s *might as well be a newtyped `Option`*
* `clippy::nonminimal_bool`: had to be worked around by introducing
extra `if`s (which should make the logic clearer)
* long-term, `nonminimal_bool` could be a real hazard, if it suggests
e.g. rewriting `!(foo && foo_bar)` into `!foo || !foo_bar`, when the
point of the `&&` is to *group* a `foo_bar` check with its `foo`
*prerequisite*
<sub>(in fact, I need to comb through Rust-GPU looking for artifacts of
this lint, because I've seen in the past conditions that were rendered
unreadable by an outer `!` being *De Morgan*'d into a `&&`, where the
outer `!` may have just been coincidental from e.g. a `retain`/`filter`
predicate's keep/remove distinction, but with `||` the correlation
between the two sides was erased...)</sub>

Fixes #13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant