diff --git a/src/cmp.rs b/src/cmp.rs index 02f6bd6..a366c45 100644 --- a/src/cmp.rs +++ b/src/cmp.rs @@ -40,29 +40,56 @@ impl Ord for Ratio { // division below, or even always avoid it for BigInt and BigUint. // FIXME- future breaking change to add Checked* to Integer? - // Compare as floored integers and remainders - let (self_int, self_rem) = self.numer.div_mod_floor(&self.denom); - let (other_int, other_rem) = other.numer.div_mod_floor(&other.denom); - match self_int.cmp(&other_int) { - Ordering::Greater => Ordering::Greater, - Ordering::Less => Ordering::Less, - Ordering::Equal => { - match (self_rem.is_zero(), other_rem.is_zero()) { - (true, true) => Ordering::Equal, - (true, false) => Ordering::Less, - (false, true) => Ordering::Greater, - (false, false) => { - // Compare the reciprocals of the remaining fractions in reverse - let self_recip = Ratio::new_raw(self.denom.clone(), self_rem); - let other_recip = Ratio::new_raw(other.denom.clone(), other_rem); - self_recip.cmp(&other_recip).reverse() - } + let (mut lhs_nbuf, mut lhs_dbuf, mut rhs_nbuf, mut rhs_dbuf); + let (mut lhs_numer, mut lhs_denom) = (&self.numer, &self.denom); + let (mut rhs_numer, mut rhs_denom) = (&other.numer, &other.denom); + loop { + match cmp_int(lhs_numer, lhs_denom, rhs_numer, rhs_denom) { + Ok(ord) => return ord, + Err((lhs_rem, rhs_rem)) => { + lhs_nbuf = lhs_rem; + lhs_numer = &lhs_nbuf; + rhs_nbuf = rhs_rem; + rhs_numer = &rhs_nbuf; + } + } + + match cmp_int(lhs_denom, lhs_numer, rhs_denom, rhs_numer) { + Ok(ord) => return ord.reverse(), + Err((lhs_rem, rhs_rem)) => { + lhs_dbuf = lhs_rem; + lhs_denom = &lhs_dbuf; + rhs_dbuf = rhs_rem; + rhs_denom = &rhs_dbuf; } } } } } +/// Compare as floored integers; if equal then return remainders +#[inline(always)] +fn cmp_int( + lhs_numer: &T, + lhs_denom: &T, + rhs_numer: &T, + rhs_denom: &T, +) -> Result { + // Compare as floored integers and remainders + let (lhs_int, lhs_rem) = lhs_numer.div_mod_floor(lhs_denom); + let (rhs_int, rhs_rem) = rhs_numer.div_mod_floor(rhs_denom); + match lhs_int.cmp(&rhs_int) { + Ordering::Greater => Ok(Ordering::Greater), + Ordering::Less => Ok(Ordering::Less), + Ordering::Equal => match (lhs_rem.is_zero(), rhs_rem.is_zero()) { + (true, true) => Ok(Ordering::Equal), + (true, false) => Ok(Ordering::Less), + (false, true) => Ok(Ordering::Greater), + (false, false) => Err((lhs_rem, rhs_rem)), + }, + } +} + impl PartialOrd for Ratio { #[inline] fn partial_cmp(&self, other: &Self) -> Option { @@ -83,15 +110,31 @@ impl Eq for Ratio {} // with `Eq` even for non-reduced ratios. impl Hash for Ratio { fn hash(&self, state: &mut H) { - recurse(&self.numer, &self.denom, state); + // This looping structure is similar to `Ord::cmp` + let (mut nbuf, mut dbuf); + let (mut numer, mut denom) = (&self.numer, &self.denom); - fn recurse(numer: &T, denom: &T, state: &mut H) { - if !denom.is_zero() { + loop { + { let (int, rem) = numer.div_mod_floor(denom); int.hash(state); - recurse(denom, &rem, state); - } else { - denom.hash(state); + if rem.is_zero() { + rem.hash(state); + return; + } + nbuf = rem; + numer = &nbuf; + } + + { + let (int, rem) = denom.div_mod_floor(numer); + int.hash(state); + if rem.is_zero() { + rem.hash(state); + return; + } + dbuf = rem; + denom = &dbuf; } } } diff --git a/src/tests.rs b/src/tests.rs index 5be1d36..5f07128 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -223,6 +223,54 @@ fn test_cmp_overflow() { } } +#[test] +#[ignore] // 2^30 combinations are slow in debug, but manageable with `--release` +fn test_cmp_i8_full() { + // All (non-reduced) `Ratio` with normal denom > 0 + let ratios = (i8::MIN..=i8::MAX) + .flat_map(|numer| (1..=i8::MAX).map(move |denom| Ratio::new_raw(numer, denom))); + for lhs in ratios.clone() { + let lhs_float = f64::from(lhs.numer) / f64::from(lhs.denom); + for rhs in ratios.clone() { + let rhs_float = f64::from(rhs.numer) / f64::from(rhs.denom); + let ord_ratio = lhs.cmp(&rhs); + let ord_float = lhs_float.partial_cmp(&rhs_float).unwrap(); + assert_eq!( + ord_ratio, ord_float, + "{lhs} ({lhs_float}) <=> {rhs} ({rhs_float})" + ); + + #[cfg(feature = "std")] + if ord_ratio.is_eq() { + assert_eq!(hash_one(&lhs), hash_one(&rhs)); + } + } + } +} + +#[cfg(feature = "num-bigint")] +fn big_ratios() -> (BigRational, BigRational) { + let mut numer = BigInt::from(29u32); + let mut denom = BigInt::from(28u32); + for _ in 0..13 { + numer = &numer * &numer; + denom = &denom * &denom; + } + let one = BigInt::one(); + let a = Ratio::new_raw(numer, denom); + let b = Ratio::new_raw(a.numer() + &one, a.denom() + &one); + (a, b) +} + +#[test] +#[cfg(feature = "num-bigint")] +fn test_cmp_stack_overflow() { + // When `cmp` was recursive, this test caused a stack overflow. (num-rational#140) + let (a, b) = big_ratios(); + assert!(a > b); + assert!(a != b); +} + #[test] fn test_to_integer() { assert_eq!(_0.to_integer(), 0); @@ -649,20 +697,20 @@ fn test_signed() { assert!(!_0.is_negative()); } -#[test] +// TODO(MSRV 1.71): use `BuildHasher::hash_one` #[cfg(feature = "std")] -fn test_hash() { - // TODO(MSRV 1.71): use `BuildHasher::hash_one` - #[cfg(feature = "std")] - fn hash_one(x: &T) -> u64 { - use std::collections::hash_map::RandomState; - use std::hash::{BuildHasher, Hasher}; +fn hash_one(x: &T) -> u64 { + use std::collections::hash_map::RandomState; + use std::hash::{BuildHasher, Hasher}; - let mut hasher = ::Hasher::new(); - x.hash(&mut hasher); - hasher.finish() - } + let mut hasher = ::Hasher::new(); + x.hash(&mut hasher); + hasher.finish() +} +#[test] +#[cfg(feature = "std")] +fn test_hash() { assert!(hash_one(&_0) != hash_one(&_1)); assert!(hash_one(&_0) != hash_one(&_3_2)); @@ -678,6 +726,16 @@ fn test_hash() { assert_eq!(hash_one(&a), hash_one(&b)); } +#[test] +#[cfg(all(feature = "std", feature = "num-bigint"))] +fn test_hash_stack_overflow() { + // When `hash` was recursive, this test caused a stack overflow. + // (related to num-rational#140) + let (a, b) = big_ratios(); + let _ = hash_one(&a); + let _ = hash_one(&b); +} + #[test] fn test_into_pair() { assert_eq!((0, 1), _0.into());