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

clippy #740

Merged
merged 2 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- run: RUSTFLAGS="--deny warnings" cargo check ${{ matrix.features }}
with:
components: clippy
- run: RUSTFLAGS="--deny warnings" cargo clippy ${{ matrix.features }}

msrv:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ categories = ["algorithms", "rust-patterns"]

edition = "2018"

# When bumping, please resolve all `#[allow(clippy::*)]` that are newly resolvable.
rust-version = "1.43.1"
Comment on lines +18 to 19
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this here. Good enough?


[lib]
Expand Down
10 changes: 9 additions & 1 deletion benches/bench1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,8 @@ fn merge_default(c: &mut Criterion) {
let mut data1 = vec![0; 1024];
let mut data2 = vec![0; 800];
let mut x = 0;

#[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]
Comment on lines +478 to +479
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this benchmark is about. I'm only guessing that calling enumerate and then ignoring the index is intentional. Same goes for the similar ones below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mightyiam After merging this PR, I search for PRs/issues about clippy (and closed some), saw #618 and eventually ran cargo clippy --all-targets and got clippy::unused_enumerate_index as "unknown lint" (yet I found it online). What am I missing here?
And second thing, we don't run clippy with --all-target and I'm wondering why, I don't see any discussion about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we run clippy four times because of a matrix value features. One of the is --all-targets --all-features.

What version toolchain did you get an error with and what is the error exactly, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh! unused_enumerate_index is added in 1.75 and I'm still on 1.74, my bad sorry! I'm so used on thinking I can't use recent features of Rust I did not thought it could be such a recent lint.
And another mistake of mine on matrix features. Ok, thanks!

for (_, elt) in data1.iter_mut().enumerate() {
*elt = x;
x += 1;
Expand All @@ -501,6 +503,8 @@ fn merge_by_cmp(c: &mut Criterion) {
let mut data1 = vec![0; 1024];
let mut data2 = vec![0; 800];
let mut x = 0;

#[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]
for (_, elt) in data1.iter_mut().enumerate() {
*elt = x;
x += 1;
Expand All @@ -527,6 +531,8 @@ fn merge_by_lt(c: &mut Criterion) {
let mut data1 = vec![0; 1024];
let mut data2 = vec![0; 800];
let mut x = 0;

#[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]
for (_, elt) in data1.iter_mut().enumerate() {
*elt = x;
x += 1;
Expand All @@ -553,6 +559,8 @@ fn kmerge_default(c: &mut Criterion) {
let mut data1 = vec![0; 1024];
let mut data2 = vec![0; 800];
let mut x = 0;

#[allow(clippy::explicit_counter_loop, clippy::unused_enumerate_index)]
for (_, elt) in data1.iter_mut().enumerate() {
*elt = x;
x += 1;
Expand Down Expand Up @@ -592,7 +600,7 @@ fn kmerge_tenway(c: &mut Criterion) {

let mut chunks = Vec::new();
let mut rest = &mut data[..];
while rest.len() > 0 {
while !rest.is_empty() {
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
let chunk_len = 1 + rng(&mut state) % 512;
let chunk_len = cmp::min(rest.len(), chunk_len as usize);
let (fst, tail) = { rest }.split_at_mut(chunk_len);
Expand Down
2 changes: 2 additions & 0 deletions benches/extra/zipslices.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ where

/// A helper trait to let `ZipSlices` accept both `&[T]` and `&mut [T]`.
///
/// # Safety
///
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
/// Unsafe trait because:
///
/// - Implementors must guarantee that `get_unchecked` is valid for all indices `0..len()`.
Expand Down
10 changes: 8 additions & 2 deletions benches/fold_specialization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,21 @@ mod specialization {
let arr = [1; 1024];

c.bench_function("internal specialized", move |b| {
b.iter(|| arr.iter().intersperse(&0).fold(0, |acc, x| acc + x))
b.iter(|| {
#[allow(clippy::unnecessary_fold)]
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
arr.iter().intersperse(&0).fold(0, |acc, x| acc + x)
})
});
}

pub fn internal_unspecialized(c: &mut Criterion) {
let arr = [1; 1024];

c.bench_function("internal unspecialized", move |b| {
b.iter(|| Unspecialized(arr.iter().intersperse(&0)).fold(0, |acc, x| acc + x))
b.iter(|| {
#[allow(clippy::unnecessary_fold)]
Unspecialized(arr.iter().intersperse(&0)).fold(0, |acc, x| acc + x)
})
});
}
}
Expand Down
4 changes: 2 additions & 2 deletions examples/iris.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::iter::repeat;
use std::num::ParseFloatError;
use std::str::FromStr;

static DATA: &'static str = include_str!("iris.data");
static DATA: &str = include_str!("iris.data");
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved

#[derive(Clone, Debug)]
struct Iris {
Expand Down Expand Up @@ -38,7 +38,7 @@ impl FromStr for Iris {
name: "".into(),
data: [0.; 4],
};
let mut parts = s.split(",").map(str::trim);
let mut parts = s.split(',').map(str::trim);
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved

// using Iterator::by_ref()
for (index, part) in parts.by_ref().take(4).enumerate() {
Expand Down
6 changes: 3 additions & 3 deletions src/adaptors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,11 +607,11 @@ where
Some(item) => Ok(f(acc, item)),
None => Err(acc),
});
let res = match res {

match res {
Ok(val) => val,
Err(val) => val,
};
res
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/adaptors/multi_product.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,9 @@ where
self.0.iter().fold(
(0, Some(0)),
|acc,
&MultiProductIter {
ref iter,
ref iter_orig,
MultiProductIter {
iter,
iter_orig,
cur: _,
}| {
let cur_size = iter.size_hint();
Expand Down
11 changes: 3 additions & 8 deletions src/either_or_both.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,13 @@ impl<A, B> EitherOrBoth<A, B> {
/// If `Left`, return true. Otherwise, return false.
/// Exclusive version of [`has_left`](EitherOrBoth::has_left).
pub fn is_left(&self) -> bool {
match *self {
Left(_) => true,
_ => false,
}
matches!(self, Left(_))
}

/// If `Right`, return true. Otherwise, return false.
/// Exclusive version of [`has_right`](EitherOrBoth::has_right).
pub fn is_right(&self) -> bool {
match *self {
Right(_) => true,
_ => false,
}
matches!(self, Right(_))
}

/// If `Both`, return true. Otherwise, return false.
Expand Down Expand Up @@ -500,6 +494,7 @@ impl<T> EitherOrBoth<T, T> {
}
}

#[allow(clippy::from_over_into)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone know why we're implementing Into instead of From?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a point of not doing it so I suggest you convert it to a From implementation in a separate commit.

EDIT: After searching, it originated in #327 and nobody said it should not be From instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an issue for that.

Now that shouldn't block this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sure want this PR to be merged as soon as possible as you made quite a work here!

impl<A, B> Into<Option<Either<A, B>>> for EitherOrBoth<A, B> {
fn into(self) -> Option<Either<A, B>> {
match self {
Expand Down
5 changes: 3 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ pub trait Itertools: Iterator {
J: IntoIterator<Item = Self::Item>,
F: FnMut(&Self::Item, &Self::Item) -> bool,
{
merge_join::merge_by_new(self, other.into_iter(), is_first)
merge_join::merge_by_new(self, other, is_first)
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
}

/// Create an iterator that merges items from both this and the specified
Expand Down Expand Up @@ -2047,6 +2047,7 @@ pub trait Itertools: Iterator {
/// let data : Option<usize> = None;
/// assert_eq!(data.into_iter().all_equal_value(), Err(None));
/// ```
#[allow(clippy::type_complexity)]
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
fn all_equal_value(&mut self) -> Result<Self::Item, Option<(Self::Item, Self::Item)>>
where
Self: Sized,
Expand Down Expand Up @@ -4013,7 +4014,7 @@ where
(None, None) => return,
(a, b) => {
let equal = match (&a, &b) {
(&Some(ref a), &Some(ref b)) => a == b,
(Some(a), Some(b)) => a == b,
_ => false,
};
assert!(
Expand Down
2 changes: 1 addition & 1 deletion src/multipeek_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ where
return None;
}
}
} else if let Some(r) = self.buf.get(0) {
} else if let Some(r) = self.buf.front() {
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
if !accept(r) {
return None;
}
Expand Down
2 changes: 1 addition & 1 deletion src/tuple_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ where
buffer
.iter()
.position(|x| x.is_none())
.unwrap_or_else(|| buffer.len())
.unwrap_or(buffer.len())
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
};
(len, Some(len))
}
Expand Down
19 changes: 9 additions & 10 deletions tests/quick.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,11 +779,11 @@ quickcheck! {

fn peek_nth_mut_replace(a: Vec<u16>, b: Vec<u16>) -> () {
let mut it = peek_nth(a.iter());
for i in 0..a.len().min(b.len()) {
*it.peek_nth_mut(i).unwrap() = &b[i];
for (i, m) in b.iter().enumerate().take(a.len().min(b.len())) {
*it.peek_nth_mut(i).unwrap() = m;
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
}
for i in 0..a.len() {
assert_eq!(it.next().unwrap(), b.get(i).unwrap_or(&a[i]));
for (i, m) in a.iter().enumerate() {
assert_eq!(it.next().unwrap(), b.get(i).unwrap_or(m));
}
assert_eq!(it.next(), None);
assert_eq!(it.next(), None);
Expand Down Expand Up @@ -875,9 +875,8 @@ quickcheck! {
quickcheck! {
fn size_put_back(a: Vec<u8>, x: Option<u8>) -> bool {
let mut it = put_back(a.into_iter());
match x {
Some(t) => it.put_back(t),
None => {}
if let Some(t) = x {
it.put_back(t)
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
}
correct_size_hint(it)
}
Expand Down Expand Up @@ -999,7 +998,7 @@ quickcheck! {
}
}
}
cmb.next() == None
cmb.next().is_none()
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -1310,7 +1309,7 @@ struct Val(u32, u32);

impl PartialOrd<Val> for Val {
fn partial_cmp(&self, other: &Val) -> Option<Ordering> {
self.0.partial_cmp(&other.0)
Some(self.cmp(other))
Philippe-Cholet marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -1413,7 +1412,7 @@ quickcheck! {
fn at_most_one_i32(a: Vec<i32>) -> TestResult {
let ret = a.iter().cloned().at_most_one();
match a.len() {
0 => TestResult::from_bool(ret.unwrap() == None),
0 => TestResult::from_bool(ret.unwrap().is_none()),
1 => TestResult::from_bool(ret.unwrap() == Some(a[0])),
_ => TestResult::from_bool(ret.unwrap_err().eq(a.iter().cloned())),
}
Expand Down
2 changes: 1 addition & 1 deletion tests/test_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ fn product2() {
assert!(prod.next() == Some(('α', 1)));
assert!(prod.next() == Some(('β', 0)));
assert!(prod.next() == Some(('β', 1)));
assert!(prod.next() == None);
assert!(prod.next().is_none());
}

#[test]
Expand Down