From 8a1967ff146255a3cf13fdbe331ce15870474d69 Mon Sep 17 00:00:00 2001 From: Emil Fresk Date: Fri, 8 Oct 2021 15:08:25 +0200 Subject: [PATCH 1/4] Generic comparison bug found --- tests/duration.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/duration.rs b/tests/duration.rs index 2137956..7b5d528 100644 --- a/tests/duration.rs +++ b/tests/duration.rs @@ -56,6 +56,29 @@ fn comparisons() { Generic::new(1_000u32, Fraction::new(1, 1_000)) == Generic::new(2_000u32, Fraction::new(1, 2_000)) ); + + // Generic comparisons + assert!( + Generic::new(100u32, Fraction::new(1, 1000)) == Generic::new(10u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(100u32, Fraction::new(1, 1000)) <= Generic::new(10u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(100u32, Fraction::new(1, 1000)) <= Generic::new(11u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(200u32, Fraction::new(1, 1000)) >= Generic::new(10u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(200u32, Fraction::new(1, 1000)) >= Generic::new(10u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(100u32, Fraction::new(1, 1000)) != Generic::new(100u32, Fraction::new(1, 100)) + ); + assert!( + Generic::new(101u32, Fraction::new(1, 1000)) > Generic::new(100u32, Fraction::new(1, 1000)) + ); } #[test] From b462581a81c6285e96c21de551122d7acf9be78d Mon Sep 17 00:00:00 2001 From: Emil Fresk Date: Fri, 8 Oct 2021 15:19:31 +0200 Subject: [PATCH 2/4] CI test --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a91e404..38298cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,7 +43,7 @@ jobs: uses: actions-rs/cargo@v1 with: command: test - args: --lib --all-features -- --test-threads=1 + args: --all-features -- --test-threads=1 - name: Test Documentation uses: actions-rs/cargo@v1 From f6db331d1fbd6c518b2df2e91fd2959a46553a00 Mon Sep 17 00:00:00 2001 From: Emil Fresk Date: Fri, 8 Oct 2021 15:54:51 +0200 Subject: [PATCH 3/4] Fixed the comparison bug --- src/duration.rs | 16 +++++++++++----- src/time_int.rs | 8 ++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/duration.rs b/src/duration.rs index 33dec7e..13af3f0 100644 --- a/src/duration.rs +++ b/src/duration.rs @@ -446,11 +446,17 @@ pub struct Generic { impl PartialOrd> for Generic { /// See [Comparisons](trait.Duration.html#comparisons) fn partial_cmp(&self, rhs: &Generic) -> Option { - Some( - self.integer - .checked_mul_fraction(&self.scaling_factor)? - .cmp(&rhs.integer.checked_mul_fraction(&rhs.scaling_factor)?), - ) + let a = self.integer; + let b = rhs.integer; + + if self.scaling_factor == rhs.scaling_factor { + Some(self.integer().cmp(&rhs.integer())) + } else { + let a = a.checked_same_base(self.scaling_factor(), rhs.scaling_factor())?; + let b = b.checked_same_base(rhs.scaling_factor(), self.scaling_factor())?; + + Some(a.cmp(&b)) + } } } diff --git a/src/time_int.rs b/src/time_int.rs index 66e1832..3a114d5 100644 --- a/src/time_int.rs +++ b/src/time_int.rs @@ -32,6 +32,14 @@ pub trait TimeInt: fn checked_div_fraction(&self, fraction: &Fraction) -> Option { self.checked_mul_fraction(&fraction.recip()) } + + /// Moves an integer into a comparable base for checking + fn checked_same_base(&self, fraction: &Fraction, rhs_fraction: &Fraction) -> Option { + let a_n = *fraction.numerator(); + let b_d = *rhs_fraction.denominator(); + + self.checked_mul(&(b_d.into()))?.checked_mul(&(a_n.into())) + } } impl TimeInt for u32 {} From 00a977274116aeb17f130faac44ca3c8fdc50b52 Mon Sep 17 00:00:00 2001 From: Peter Taylor Date: Sun, 17 Oct 2021 13:57:23 -0600 Subject: [PATCH 4/4] refactor(duration): adjust Generic duration `PartialOrd` impl to eliminate overrun failures --- src/duration.rs | 44 +++++++++++++++++++++++++++++++++----------- tests/duration.rs | 24 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/duration.rs b/src/duration.rs index 13af3f0..d7e3d45 100644 --- a/src/duration.rs +++ b/src/duration.rs @@ -443,19 +443,41 @@ pub struct Generic { scaling_factor: Fraction, } +impl Generic { + /// Try to create a new, equivalent `Generic` with the given _scaling factor_ + fn try_into_scaling_factor(self, scaling_factor: Fraction) -> Result { + let new_int = TimeInt::checked_mul_fraction( + &self.integer, + &self + .scaling_factor + .checked_div(&scaling_factor) + .ok_or(ConversionError::Overflow)?, + ) + .ok_or(ConversionError::Overflow)?; + + Ok(Self::new(new_int, scaling_factor)) + } +} + impl PartialOrd> for Generic { /// See [Comparisons](trait.Duration.html#comparisons) fn partial_cmp(&self, rhs: &Generic) -> Option { - let a = self.integer; - let b = rhs.integer; - if self.scaling_factor == rhs.scaling_factor { - Some(self.integer().cmp(&rhs.integer())) + Some(self.integer.cmp(&rhs.integer)) + } else if self.scaling_factor < rhs.scaling_factor { + // convert to the smaller scaling factor (rhs -> self) + // if conversion fails, we know self is less than rhs + match rhs.try_into_scaling_factor(self.scaling_factor) { + Ok(converted_rhs) => Some(self.integer.cmp(&converted_rhs.integer)), + Err(_) => Some(core::cmp::Ordering::Less), + } } else { - let a = a.checked_same_base(self.scaling_factor(), rhs.scaling_factor())?; - let b = b.checked_same_base(rhs.scaling_factor(), self.scaling_factor())?; - - Some(a.cmp(&b)) + // convert to the smaller scaling factor (self -> rhs) + // if conversion fails, we know rhs is less than self + match self.try_into_scaling_factor(rhs.scaling_factor) { + Ok(converted_self) => Some(converted_self.integer.cmp(&rhs.integer)), + Err(_) => Some(core::cmp::Ordering::Greater), + } } } } @@ -890,9 +912,9 @@ pub mod units { /// See [Comparisons](trait.Duration.html#comparisons) fn partial_cmp(&self, rhs: &$big) -> Option { match Self::try_from(*rhs) { - Ok(rhs) => Some(self.integer().cmp(&rhs.integer())), - Err(_) => Some(core::cmp::Ordering::Less), - } + Ok(rhs) => Some(self.integer().cmp(&rhs.integer())), + Err(_) => Some(core::cmp::Ordering::Less), + } } } )+ diff --git a/tests/duration.rs b/tests/duration.rs index 7b5d528..4e8dff7 100644 --- a/tests/duration.rs +++ b/tests/duration.rs @@ -79,6 +79,30 @@ fn comparisons() { assert!( Generic::new(101u32, Fraction::new(1, 1000)) > Generic::new(100u32, Fraction::new(1, 1000)) ); + assert!( + Generic::new(u32::MAX, Fraction::new(1, 100)) + > Generic::new(100u32, Fraction::new(1, 1000)) + ); + + /* Generic comparison stress test: + loop { + let mut rands: [u32; 6] = [0; 6]; + for rand in rands.iter_mut() { + unsafe { + loop { + core::arch::x86_64::_rdrand32_step(rand); + if *rand != 0 { + break; + } + } + } + } + println!("rands: {:#?}", rands); + assert!( + Generic::new(rands[0], Fraction::new(rands[1], rands[2])) + != Generic::new(rands[3], Fraction::new(rands[4], rands[5])) + ); + } */ } #[test]