From d1a25d68728c088c8fa823d89183284245e132cc Mon Sep 17 00:00:00 2001 From: "Brian L. Troutwine" Date: Sat, 1 Aug 2020 15:19:44 -0700 Subject: [PATCH] Introduce clippy changes This commit introduces a set of modern clippy lint fixes. The majority are simple mechanical adjustments, though a few involve more proper checking of bounds in our quickcheck tests. --- benches/ckms.rs | 2 +- src/ckms/mod.rs | 56 +++++++++++++++--------------- src/ckms/store.rs | 17 ++++----- src/greenwald_khanna.rs | 76 ++++++++++++++++++----------------------- src/histogram.rs | 27 ++++++++------- 5 files changed, 86 insertions(+), 92 deletions(-) diff --git a/benches/ckms.rs b/benches/ckms.rs index 9c073a9..6c59c00 100644 --- a/benches/ckms.rs +++ b/benches/ckms.rs @@ -11,7 +11,7 @@ mod ckms { impl Xorshift { pub fn new(seed: u64) -> Xorshift { - Xorshift { seed: seed } + Xorshift { seed } } pub fn next_val(&mut self) -> u32 { diff --git a/src/ckms/mod.rs b/src/ckms/mod.rs index 609df32..975d715 100644 --- a/src/ckms/mod.rs +++ b/src/ckms/mod.rs @@ -135,7 +135,7 @@ impl< CKMS { n: 0, - insert_threshold: insert_threshold, + insert_threshold, inserts: 0, samples: Store::new(2048, error), @@ -295,19 +295,18 @@ mod test { use super::*; use ckms::store::invariant; use quickcheck::{QuickCheck, TestResult}; + use std::cmp::Ordering; use std::f64::consts::E; - fn percentile(data: &Vec, prcnt: f64) -> f64 { + fn percentile(data: &[f64], prcnt: f64) -> f64 { let idx = (prcnt * (data.len() as f64)) as usize; - return data[idx]; + data[idx] } #[test] fn test_cma() { fn inner(data: Vec, err: f64) -> TestResult { - if data.is_empty() { - return TestResult::discard(); - } else if !(err >= 0.0) || !(err <= 1.0) { + if !between_inclusive(err, 0.0, 1.0) || data.is_empty() { return TestResult::discard(); } @@ -322,7 +321,7 @@ mod test { assert!(mean.is_some()); assert!((expected_mean - mean.unwrap()).abs() < err); - return TestResult::passed(); + TestResult::passed() } QuickCheck::new().quickcheck(inner as fn(Vec, f64) -> TestResult); } @@ -330,7 +329,7 @@ mod test { #[test] fn test_cma_add_assign() { fn inner(l_data: Vec, r_data: Vec, err: f64) -> TestResult { - if !(err >= 0.0) || !(err <= 1.0) { + if !between_inclusive(err, 0.0, 1.0) { return TestResult::discard(); } @@ -350,7 +349,7 @@ mod test { if mean.is_some() { assert!((expected_mean - mean.unwrap()).abs() < err); } - return TestResult::passed(); + TestResult::passed() } QuickCheck::new().quickcheck(inner as fn(Vec, Vec, f64) -> TestResult); } @@ -359,9 +358,7 @@ mod test { fn error_nominal_test() { fn inner(mut data: Vec, prcnt: f64) -> TestResult { data.sort_by(|a, b| a.partial_cmp(b).unwrap()); - if !(prcnt >= 0.0) || !(prcnt <= 1.0) { - return TestResult::discard(); - } else if data.len() < 1 { + if !between_inclusive(prcnt, 0.0, 1.0) || data.is_empty() { return TestResult::discard(); } let err = 0.001; @@ -388,17 +385,22 @@ mod test { QuickCheck::new().quickcheck(inner as fn(Vec, f64) -> TestResult); } + pub fn between_inclusive(val: f64, lower: f64, upper: f64) -> bool { + match val.partial_cmp(&lower) { + None | Some(Ordering::Less) => false, + _ => !matches!(val.partial_cmp(&upper), None | Some(Ordering::Greater)), + } + } + #[test] fn error_nominal_with_merge_test() { fn inner(lhs: Vec, rhs: Vec, prcnt: f64, err: f64) -> TestResult { - if !(prcnt >= 0.0) || !(prcnt <= 1.0) { - return TestResult::discard(); - } else if !(err >= 0.0) || !(err <= 1.0) { - return TestResult::discard(); - } else if (lhs.len() + rhs.len()) < 1 { - return TestResult::discard(); - } - if lhs.is_empty() || rhs.is_empty() { + if !between_inclusive(prcnt, 0.0, 1.0) + || !between_inclusive(err, 0.0, 1.0) + || (lhs.len() + rhs.len()) < 1 + || lhs.is_empty() + || rhs.is_empty() + { return TestResult::discard(); } let mut data = lhs.clone(); @@ -476,7 +478,7 @@ mod test { fn query_invariant_test() { fn query_invariant(f: f64, fs: Vec) -> TestResult { let error = 0.001; - if fs.len() < 1 { + if fs.is_empty() { return TestResult::discard(); } @@ -509,8 +511,8 @@ mod test { ckms.insert(i as f64); } - assert_eq!(0.0, ckms.samples[0].v); - assert_eq!(1.0, ckms.samples[1].v); + assert!((0.0 - ckms.samples[0].v).abs() < f64::EPSILON); + assert!((1.0 - ckms.samples[1].v).abs() < f64::EPSILON); } // prop: v_i-1 < v_i =< v_i+1 @@ -523,7 +525,7 @@ mod test { ckms.insert(f); } - if ckms.samples.len() == 0 && fsc.len() == 0 { + if ckms.samples.is_empty() && fsc.is_empty() { return TestResult::passed(); } let mut cur = ckms.samples[0].v; @@ -552,8 +554,8 @@ mod test { let s = ckms.samples.len(); let mut r = 0; for i in 1..s { - let ref prev = ckms.samples[i - 1]; - let ref cur = ckms.samples[i]; + let prev = &ckms.samples[i - 1]; + let cur = &ckms.samples[i]; r += prev.g; @@ -608,7 +610,7 @@ mod test { // We have to choose an arbitrary, lowish constant for bound // invalidation buffer. This is because I don't have a precise // boundary. 1024 samples worth of slop isn't bad, I guess. - if !(s <= bound) && !((s - bound).abs() < 1_024) { + if s > bound && (s - bound).abs() >= 1_024 { println!( "error: {:?} n: {:?} log10: {:?}", ckms.error_bound(), diff --git a/src/ckms/store.rs b/src/ckms/store.rs index 24037c5..478c4bd 100644 --- a/src/ckms/store.rs +++ b/src/ckms/store.rs @@ -110,16 +110,16 @@ where g_sum: 0, }; Store { - error: error, + error, data: vec![data], - inner_cap: inner_cap, + inner_cap, len: 0, n: 0, } } /// Insert a point into the Store - pub fn insert(&mut self, element: T) -> () + pub fn insert(&mut self, element: T) where T: fmt::Debug, { @@ -319,7 +319,7 @@ where // through the inner caches and combine those that are contiguous and // fit within inner_cap. cur_outer_idx = 0; - while (self.data.len() >= 1) && (cur_outer_idx < (self.data.len() - 1)) { + while !self.data.is_empty() && (cur_outer_idx < (self.data.len() - 1)) { if self.data[cur_outer_idx].data.len() + self.data[cur_outer_idx + 1].data.len() <= self.inner_cap { @@ -459,6 +459,7 @@ where #[cfg(test)] mod test { use super::*; + use ckms::test::between_inclusive; use quickcheck::{QuickCheck, TestResult}; #[test] @@ -486,11 +487,7 @@ mod test { #[test] fn obey_inner_cap() { fn inner(data: Vec, inner_cap: usize, err: f64) -> TestResult { - if data.is_empty() { - return TestResult::discard(); - } else if inner_cap == 0 { - return TestResult::discard(); - } else if !(err >= 0.0) || !(err <= 1.0) { + if data.is_empty() || inner_cap == 0 || !between_inclusive(err, 0.0, 1.0) { return TestResult::discard(); } @@ -503,7 +500,7 @@ mod test { assert!(inner.len() <= store.inner_cap); } - return TestResult::passed(); + TestResult::passed() } QuickCheck::new().quickcheck(inner as fn(Vec, usize, f64) -> TestResult); } diff --git a/src/greenwald_khanna.rs b/src/greenwald_khanna.rs index a5b4e91..980c806 100644 --- a/src/greenwald_khanna.rs +++ b/src/greenwald_khanna.rs @@ -141,11 +141,7 @@ where { /// Creates a new instance of a Tuple pub fn new(v: T, g: usize, delta: usize) -> Tuple { - Tuple { - v: v, - g: g, - delta: delta, - } + Tuple { v, g, delta } } } @@ -191,7 +187,7 @@ where pub fn new(epsilon: f64) -> Stream { Stream { summary: vec![], - epsilon: epsilon, + epsilon, n: 0, } } @@ -221,7 +217,7 @@ where /// Compute the epsilon-approximate phi-quantile /// from the summary data structure. pub fn quantile(&self, phi: f64) -> &T { - assert!(self.summary.len() >= 1); + assert!(!self.summary.is_empty()); assert!(phi >= 0f64 && phi <= 1f64); let r = (phi * self.n as f64).floor() as usize; @@ -358,42 +354,38 @@ impl AddAssign for Stream { let mut rhs_samples = rhs.summary.into_iter().peekable(); let mut started_self = false; let mut started_rhs = false; - loop { - match (self_samples.peek(), rhs_samples.peek()) { - (Some(self_sample), Some(rhs_sample)) => { - // Detect next sample - let (next_sample, additional_delta) = if self_sample.v < rhs_sample.v { - started_self = true; - ( - self_samples.next().unwrap(), - if started_rhs { - additional_self_delta - } else { - 0 - }, - ) + while let (Some(self_sample), Some(rhs_sample)) = (self_samples.peek(), rhs_samples.peek()) + { + // Detect next sample + let (next_sample, additional_delta) = if self_sample.v < rhs_sample.v { + started_self = true; + ( + self_samples.next().unwrap(), + if started_rhs { + additional_self_delta } else { - started_rhs = true; - ( - rhs_samples.next().unwrap(), - if started_self { - additional_rhs_delta - } else { - 0 - }, - ) - }; - - // Insert it - let next_sample = Tuple { - v: next_sample.v, - g: next_sample.g, - delta: next_sample.delta + additional_delta, - }; - merged_summary.push(next_sample); - } - _ => break, - } + 0 + }, + ) + } else { + started_rhs = true; + ( + rhs_samples.next().unwrap(), + if started_self { + additional_rhs_delta + } else { + 0 + }, + ) + }; + + // Insert it + let next_sample = Tuple { + v: next_sample.v, + g: next_sample.g, + delta: next_sample.delta + additional_delta, + }; + merged_summary.push(next_sample); } // Copy the remaining samples from the rhs list diff --git a/src/histogram.rs b/src/histogram.rs index fea802b..17dc462 100644 --- a/src/histogram.rs +++ b/src/histogram.rs @@ -204,7 +204,7 @@ where Ok(Histogram { count: 0, sum: None, - bins: bins, + bins, }) } @@ -225,7 +225,7 @@ where /// assert_eq!(histo.total_between(Bound::Finite(10), Bound::Finite(100)), /// 2); /// ``` - pub fn insert(&mut self, value: T) -> () + pub fn insert(&mut self, value: T) where T: ops::Add, { @@ -452,13 +452,16 @@ mod test { if res.sum().is_some() { match (x.sum().is_some(), y.sum().is_some()) { (true, true) => { - assert_eq!(res.sum().unwrap(), x.sum().unwrap() + y.sum().unwrap()); + assert!( + (res.sum().unwrap() - (x.sum().unwrap() + y.sum().unwrap())).abs() + < f64::EPSILON + ); } (false, true) => { - assert_eq!(res.sum().unwrap(), y.sum().unwrap()); + assert!((res.sum().unwrap() - y.sum().unwrap()).abs() < f64::EPSILON); } (true, false) => { - assert_eq!(res.sum().unwrap(), x.sum().unwrap()); + assert!((res.sum().unwrap() - x.sum().unwrap()).abs() < f64::EPSILON); } (false, false) => unreachable!(), } @@ -561,7 +564,7 @@ mod test { } let mut bounds: Vec> = - bounds.into_iter().map(|x| Bound::Finite(x)).collect(); + bounds.into_iter().map(Bound::Finite).collect(); bounds.push(Bound::PosInf); // confirm that the histogram has correctly binned by @@ -572,14 +575,14 @@ mod test { let mut below_count = 0; for v in pyld.iter() { match b { - &Bound::Finite(ref bnd) => { + Bound::Finite(ref bnd) => { if v <= bnd { below_count += 1; } else { break; } } - &Bound::PosInf => { + Bound::PosInf => { below_count += 1; } } @@ -606,7 +609,7 @@ mod test { } let mut bounds: Vec> = - bounds.into_iter().map(|x| Bound::Finite(x)).collect(); + bounds.into_iter().map(Bound::Finite).collect(); bounds.push(Bound::PosInf); // confirm that the histogram has correctly binned by @@ -617,12 +620,12 @@ mod test { let mut above_count = 0; for v in pyld.iter() { match b { - &Bound::Finite(ref bnd) => { + Bound::Finite(ref bnd) => { if v > bnd { above_count += 1; } } - &Bound::PosInf => {} + Bound::PosInf => {} } } assert_eq!(above_count, histo.total_above(*b)) @@ -647,7 +650,7 @@ mod test { } let mut bounds: Vec> = - bounds.into_iter().map(|x| Bound::Finite(x)).collect(); + bounds.into_iter().map(Bound::Finite).collect(); bounds.push(Bound::PosInf); // confirm that the histogram has correctly binned by