From 027feee15ee19bdc7a1cccbc369995c7f6f0621a Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Tue, 16 Apr 2019 03:14:12 -0700 Subject: [PATCH 01/14] WIP rewrite Add for Ratio to prevent overflow Tries to fix #13 by using least common multiple (LCM) for denominator when summing Ratios. i.e. 1/255 + 1/255 -> (1+1)/LCM(255,255) -> 2/255 instead of 1/255 + 1/255 -> (1*255 + 1*255)/(255*255), which would cause an overflow if the integer type is u8. Also adds minmal tests making sure 1/u8::max_value() + 1/u8::max_value() doesn't overflow for all integer types --- src/lib.rs | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 044d2a9..0bb46e8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -743,6 +743,31 @@ where } } + +forward_all_binop!(impl Add, add); + +impl Add> for Ratio { + type Output = Ratio; + #[inline] + // a/b + c/d = (a*lcm/b + c*lcm/d)/lcm where lcm = lcm(b,d) + fn add(self, rhs: Ratio) -> Ratio { + let lcm = self.denom.lcm(&rhs.denom.clone()); + let lhs_numer = self.numer * (lcm.clone() / self.denom); + let rhs_numer = rhs.numer * (lcm.clone() / rhs.denom); + + Ratio::new(lhs_numer + rhs_numer, lcm.clone()) + } +} + +impl Add for Ratio { + type Output = Ratio; + // a/b + c/1 = (a + c*b)/b + #[inline] + fn add(self: Ratio, rhs: T) -> Ratio { + Ratio::new(self.numer + (self.denom.clone() * rhs), self.denom) + } +} + macro_rules! arith_impl { (impl $imp:ident, $method:ident) => { forward_all_binop!(impl $imp, $method); @@ -768,7 +793,6 @@ macro_rules! arith_impl { }; } -arith_impl!(impl Add, add); arith_impl!(impl Sub, sub); arith_impl!(impl Rem, rem); @@ -1538,9 +1562,11 @@ mod test { mod arith { use super::super::{Ratio, Rational}; - use super::{_0, _1, _1_2, _2, _3_2, _NEG1_2, to_big}; - use traits::{CheckedAdd, CheckedDiv, CheckedMul, CheckedSub}; - + use super::{to_big, _0, _1, _1_2, _2, _3_2, _NEG1_2}; + use core::fmt::Debug; + use integer::Integer; + use traits::NumCast; + use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub}; #[test] fn test_add() { fn test(a: Rational, b: Rational, c: Rational) { @@ -1576,6 +1602,48 @@ mod test { test_assign(_1_2, 1, _3_2); } + #[test] + fn test_add_overflow() { + + //As a concrete example for u8, which has a range of values from 0 to 255 + //compares Ratio(1, 255) + Ratio(1, 255) to Ratio (1+1, 255) + //with old behavior, Ratio(1, 255) + Ratio(1, 255) would cause an overflow + fn test_add_typed_overflow() + where + T: Integer + Bounded + NumCast + Clone + Debug, + { + let _6 = T::one() + T::one() + T::one() + T::one() + T::one() + T::one(); + let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); + let _2_max = Ratio::new(T::one() + T::one(), T::max_value()); + let _6_max = Ratio::new(_6, T::max_value()); + assert_eq!(_1_max.clone() + _1_max.clone(), _2_max); + assert_eq!( + _1_max.clone() + + _1_max.clone() + + _1_max.clone() + + _1_max.clone() + + _1_max.clone() + + _1_max.clone(), + _6_max + ); + } + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + #[cfg(has_u128)] + test_add_typed_overflow::(); + + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + test_add_typed_overflow::(); + #[cfg(has_i128)] + test_add_typed_overflow::(); + } + #[test] fn test_sub() { fn test(a: Rational, b: Rational, c: Rational) { From f78af66d5bf72e539010b22a7bc81505940a8ea3 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Thu, 18 Apr 2019 01:56:51 -0700 Subject: [PATCH 02/14] fix #13 implements least common multiple rational {addition, subtraction, modulo} using the macro present in previous versions of this library --- src/lib.rs | 104 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 36 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0bb46e8..9ceb052 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -743,43 +743,18 @@ where } } - -forward_all_binop!(impl Add, add); - -impl Add> for Ratio { - type Output = Ratio; - #[inline] - // a/b + c/d = (a*lcm/b + c*lcm/d)/lcm where lcm = lcm(b,d) - fn add(self, rhs: Ratio) -> Ratio { - let lcm = self.denom.lcm(&rhs.denom.clone()); - let lhs_numer = self.numer * (lcm.clone() / self.denom); - let rhs_numer = rhs.numer * (lcm.clone() / rhs.denom); - - Ratio::new(lhs_numer + rhs_numer, lcm.clone()) - } -} - -impl Add for Ratio { - type Output = Ratio; - // a/b + c/1 = (a + c*b)/b - #[inline] - fn add(self: Ratio, rhs: T) -> Ratio { - Ratio::new(self.numer + (self.denom.clone() * rhs), self.denom) - } -} - macro_rules! arith_impl { (impl $imp:ident, $method:ident) => { forward_all_binop!(impl $imp, $method); - // Abstracts the a/b `op` c/d = (a*d `op` b*c) / (b*d) pattern + // Abstracts a/b `op` c/d = (a*lcm/b `op` c*lcm/d)/lcm where lcm = lcm(b,d) impl $imp> for Ratio { type Output = Ratio; #[inline] fn $method(self, rhs: Ratio) -> Ratio { - Ratio::new( - (self.numer * rhs.denom.clone()).$method(self.denom.clone() * rhs.numer), - self.denom * rhs.denom, - ) + let lcm = self.denom.lcm(&rhs.denom.clone()); + let lhs_numer = self.numer * (lcm.clone() / self.denom); + let rhs_numer = rhs.numer * (lcm.clone() / rhs.denom); + Ratio::new(lhs_numer.$method(rhs_numer), lcm) } } // Abstracts the a/b `op` c/1 = (a*1 `op` b*c) / (b*1) = (a `op` b*c) / b pattern @@ -793,6 +768,7 @@ macro_rules! arith_impl { }; } +arith_impl!(impl Add, add); arith_impl!(impl Sub, sub); arith_impl!(impl Rem, rem); @@ -1565,8 +1541,8 @@ mod test { use super::{to_big, _0, _1, _1_2, _2, _3_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; - use traits::NumCast; use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub}; + #[test] fn test_add() { fn test(a: Rational, b: Rational, c: Rational) { @@ -1604,13 +1580,12 @@ mod test { #[test] fn test_add_overflow() { - - //As a concrete example for u8, which has a range of values from 0 to 255 - //compares Ratio(1, 255) + Ratio(1, 255) to Ratio (1+1, 255) - //with old behavior, Ratio(1, 255) + Ratio(1, 255) would cause an overflow + // compares Ratio(1, T::max_value()) + Ratio(1, T::max_value()) + // to Ratio(1+1, T::max_value()) for each integer type. + // Previously, this calculation would overflow. fn test_add_typed_overflow() where - T: Integer + Bounded + NumCast + Clone + Debug, + T: Integer + Bounded + Clone + Debug, { let _6 = T::one() + T::one() + T::one() + T::one() + T::one() + T::one(); let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); @@ -1678,6 +1653,34 @@ mod test { test_assign(_1_2, 1, _NEG1_2); } + #[test] + fn test_sub_overflow() { + // compares Ratio(1, T::max_value()) - Ratio(1, T::max_value()) to T::zero() + // for each integer type. Previously, this calculation would overflow. + fn test_sub_typed_overflow() + where + T: Integer + Bounded + Clone + Debug, + { + let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); + assert!(T::is_zero(&(_1_max.clone() - _1_max.clone()).numer)); + } + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + #[cfg(has_u128)] + test_add_typed_overflow::(); + + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + test_sub_typed_overflow::(); + #[cfg(has_i128)] + test_sub_typed_overflow::(); + } + #[test] fn test_mul() { fn test(a: Rational, b: Rational, c: Rational) { @@ -1778,6 +1781,35 @@ mod test { test_assign(_3_2, 1, _1_2); } + #[test] + fn test_rem_overflow() { + // tests that Ratio(1,1) % Ratio(1, T::max_value()) equals 0 + // for each integer type. Previously, this calculation would overflow. + fn test_rem_typed_overflow() + where + T: Integer + Bounded + Clone + Debug, + { + let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); + let _1_1: Ratio = Ratio::new(T::one(), T::one()); + assert!(T::is_zero(&(_1_1 % _1_max.clone()).numer)); + } + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + #[cfg(has_u128)] + test_rem_typed_overflow::(); + + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + test_rem_typed_overflow::(); + #[cfg(has_i128)] + test_rem_typed_overflow::(); + } + #[test] fn test_neg() { fn test(a: Rational, b: Rational) { From 3f7f5859d73e323fac94436e9981a77d2e4070ab Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Fri, 19 Apr 2019 01:43:55 -0700 Subject: [PATCH 03/14] fix #13 for checked_add and checked_multiply --- src/lib.rs | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9ceb052..5d2050f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -819,19 +819,20 @@ macro_rules! checked_arith_impl { impl $imp for Ratio { #[inline] fn $method(&self, rhs: &Ratio) -> Option> { - let ad = otry!(self.numer.checked_mul(&rhs.denom)); - let bc = otry!(self.denom.checked_mul(&rhs.numer)); - let bd = otry!(self.denom.checked_mul(&rhs.denom)); - Some(Ratio::new(otry!(ad.$method(&bc)), bd)) + let gcd = self.denom.clone().gcd(&rhs.denom.clone()); + let lcm = (self.denom.clone() / gcd.clone()).checked_mul(&rhs.denom)?; + let lhs_numer = (lcm.clone() / self.denom.clone()).checked_mul(&self.numer)?; + let rhs_numer = (lcm.clone() / rhs.denom.clone()).checked_mul(&rhs.numer)?; + Some(Ratio::new(lhs_numer.$method(&rhs_numer)?, lcm)) } } }; } -// a/b + c/d = (a*d + b*c)/(b*d) +// a/b + c/d = (lcm/b*a + lcm/d*c)/lcm, where lcm = lcm(b,d) checked_arith_impl!(impl CheckedAdd, checked_add); -// a/b - c/d = (a*d - b*c)/(b*d) +// a/b - c/d = (lcm/b*a - lcm/d*c)/lcm, where lcm = lcm(b,d) checked_arith_impl!(impl CheckedSub, checked_sub); impl Neg for Ratio @@ -1585,13 +1586,14 @@ mod test { // Previously, this calculation would overflow. fn test_add_typed_overflow() where - T: Integer + Bounded + Clone + Debug, + T: Integer + Bounded + Clone + Debug + CheckedAdd + CheckedMul, { let _6 = T::one() + T::one() + T::one() + T::one() + T::one() + T::one(); - let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); + let _1_max = Ratio::new(T::one(), T::max_value()); let _2_max = Ratio::new(T::one() + T::one(), T::max_value()); let _6_max = Ratio::new(_6, T::max_value()); assert_eq!(_1_max.clone() + _1_max.clone(), _2_max); + assert_eq!(_1_max.clone().checked_add(&_1_max), Some(_2_max.clone())); assert_eq!( _1_max.clone() + _1_max.clone() @@ -1601,6 +1603,20 @@ mod test { + _1_max.clone(), _6_max ); + assert_eq!( + _1_max + .clone() + .checked_add(&_1_max) + .unwrap() + .checked_add(&_1_max) + .unwrap() + .checked_add(&_1_max) + .unwrap() + .checked_add(&_1_max) + .unwrap() + .checked_add(&_1_max), + Some(_6_max.clone()) + ); } test_add_typed_overflow::(); test_add_typed_overflow::(); @@ -1659,10 +1675,13 @@ mod test { // for each integer type. Previously, this calculation would overflow. fn test_sub_typed_overflow() where - T: Integer + Bounded + Clone + Debug, + T: Integer + Bounded + Clone + Debug + CheckedSub + CheckedMul, { let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); assert!(T::is_zero(&(_1_max.clone() - _1_max.clone()).numer)); + assert!(T::is_zero( + &(_1_max.clone().checked_sub(&_1_max).unwrap().numer) + )); } test_sub_typed_overflow::(); test_sub_typed_overflow::(); From 5439dcd53896b81cd999cc0443d1fa6cd9bc3c90 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Fri, 19 Apr 2019 02:06:52 -0700 Subject: [PATCH 04/14] replace ? with otry! to mantain rust 1.15 compatibility --- src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 5d2050f..744b642 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -820,10 +820,10 @@ macro_rules! checked_arith_impl { #[inline] fn $method(&self, rhs: &Ratio) -> Option> { let gcd = self.denom.clone().gcd(&rhs.denom.clone()); - let lcm = (self.denom.clone() / gcd.clone()).checked_mul(&rhs.denom)?; - let lhs_numer = (lcm.clone() / self.denom.clone()).checked_mul(&self.numer)?; - let rhs_numer = (lcm.clone() / rhs.denom.clone()).checked_mul(&rhs.numer)?; - Some(Ratio::new(lhs_numer.$method(&rhs_numer)?, lcm)) + let lcm = otry!((self.denom.clone() / gcd.clone()).checked_mul(&rhs.denom)); + let lhs_numer = otry!((lcm.clone() / self.denom.clone()).checked_mul(&self.numer)); + let rhs_numer = otry!((lcm.clone() / rhs.denom.clone()).checked_mul(&rhs.numer)); + Some(Ratio::new(otry!(lhs_numer.$method(&rhs_numer)), lcm)) } } }; From 2859a2cd647ab5d3de9a401fe38de45364e370d6 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Fri, 14 Jun 2019 13:36:56 -0700 Subject: [PATCH 05/14] fix #13 for AddAssign, RemAssign and SubAssign --- src/lib.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 744b642..8e97838 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -506,9 +506,11 @@ mod opassign { impl AddAssign for Ratio { fn add_assign(&mut self, other: Ratio) { - self.numer *= other.denom.clone(); - self.numer += self.denom.clone() * other.numer; - self.denom *= other.denom; + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer + rhs_numer; + self.denom = lcm; self.reduce(); } } @@ -531,18 +533,22 @@ mod opassign { impl RemAssign for Ratio { fn rem_assign(&mut self, other: Ratio) { - self.numer *= other.denom.clone(); - self.numer %= self.denom.clone() * other.numer; - self.denom *= other.denom; + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer % rhs_numer; + self.denom = lcm; self.reduce(); } } impl SubAssign for Ratio { fn sub_assign(&mut self, other: Ratio) { - self.numer *= other.denom.clone(); - self.numer -= self.denom.clone() * other.numer; - self.denom *= other.denom; + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer - rhs_numer; + self.denom = lcm; self.reduce(); } } From f82341e1abdfcece1abce87bdb175efd717b9138 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Fri, 14 Jun 2019 15:18:20 -0700 Subject: [PATCH 06/14] special case equal denominators for +,-,%,+=,-=,%= --- src/lib.rs | 50 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8e97838..ecba709 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -506,11 +506,15 @@ mod opassign { impl AddAssign for Ratio { fn add_assign(&mut self, other: Ratio) { - let lcm = self.denom.lcm(&other.denom.clone()); - let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); - let rhs_numer = other.numer * (lcm.clone() / other.denom); - self.numer = lhs_numer + rhs_numer; - self.denom = lcm; + if self.denom == other.denom { + self.numer += other.numer + } else { + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer + rhs_numer; + self.denom = lcm; + } self.reduce(); } } @@ -533,22 +537,30 @@ mod opassign { impl RemAssign for Ratio { fn rem_assign(&mut self, other: Ratio) { - let lcm = self.denom.lcm(&other.denom.clone()); - let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); - let rhs_numer = other.numer * (lcm.clone() / other.denom); - self.numer = lhs_numer % rhs_numer; - self.denom = lcm; + if self.denom == other.denom { + self.numer %= other.numer + } else { + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer % rhs_numer; + self.denom = lcm; + } self.reduce(); } } impl SubAssign for Ratio { fn sub_assign(&mut self, other: Ratio) { - let lcm = self.denom.lcm(&other.denom.clone()); - let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); - let rhs_numer = other.numer * (lcm.clone() / other.denom); - self.numer = lhs_numer - rhs_numer; - self.denom = lcm; + if self.denom == other.denom { + self.numer -= other.numer + } else { + let lcm = self.denom.lcm(&other.denom.clone()); + let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); + let rhs_numer = other.numer * (lcm.clone() / other.denom); + self.numer = lhs_numer - rhs_numer; + self.denom = lcm; + } self.reduce(); } } @@ -757,6 +769,9 @@ macro_rules! arith_impl { type Output = Ratio; #[inline] fn $method(self, rhs: Ratio) -> Ratio { + if self.denom == rhs.denom { + return Ratio::new(self.numer.$method(rhs.numer), rhs.denom); + } let lcm = self.denom.lcm(&rhs.denom.clone()); let lhs_numer = self.numer * (lcm.clone() / self.denom); let rhs_numer = rhs.numer * (lcm.clone() / rhs.denom); @@ -1337,6 +1352,7 @@ mod test { }; pub const _1_2: Rational = Ratio { numer: 1, denom: 2 }; pub const _3_2: Rational = Ratio { numer: 3, denom: 2 }; + pub const _5_2: Rational = Ratio { numer: 5, denom: 2 }; pub const _NEG1_2: Rational = Ratio { numer: -1, denom: 2, @@ -1545,7 +1561,7 @@ mod test { mod arith { use super::super::{Ratio, Rational}; - use super::{to_big, _0, _1, _1_2, _2, _3_2, _NEG1_2}; + use super::{to_big, _0, _1, _1_2, _2, _3_2, _5_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub}; @@ -1801,6 +1817,8 @@ mod test { } test(_3_2, _1, _1_2); + test(_3_2, _1_2, _0); + test(_5_2, _3_2, _1); test(_2, _NEG1_2, _0); test(_1_2, _2, _1_2); test_assign(_3_2, 1, _1_2); From 5579400219adef5e94d053251ab8e56cdf6c785e Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Sun, 16 Jun 2019 11:05:55 -0700 Subject: [PATCH 07/14] fix #13 for *,*=,/,/=; add overflow tests for these operators --- src/lib.rs | 152 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 136 insertions(+), 16 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ecba709..791d623 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -519,19 +519,31 @@ mod opassign { } } + // (a/b) / (c/d) = (a/gcd1)*(d/gcd2) / ((c/gcd1)*(b/gcd2)) + // where gcd1 = gcd(a,c); gcd2 = gcd(b,d) impl DivAssign for Ratio { fn div_assign(&mut self, other: Ratio) { - self.numer *= other.denom; - self.denom *= other.numer; - self.reduce(); + let numer_gcd = self.numer.gcd(&other.numer.clone()); + let denom_gcd = self.denom.gcd(&other.denom.clone()); + self.numer /= numer_gcd.clone(); + self.numer *= other.denom / denom_gcd.clone(); + self.denom /= denom_gcd.clone(); + self.denom *= other.numer / numer_gcd.clone(); + self.reduce(); //TODO: remove this line. see #8. } } + // a/b * c/d = (a/gcd1)*(c/gcd2) / ((d/gcd1)*(b/gcd2)) + // where gcd1 = gcd(a,d); gcd2 = gcd(b,c) impl MulAssign for Ratio { fn mul_assign(&mut self, other: Ratio) { - self.numer *= other.numer; - self.denom *= other.denom; - self.reduce(); + let gcd1 = self.numer.gcd(&other.denom.clone()); + let gcd2 = self.denom.gcd(&other.numer.clone()); + self.numer /= gcd1.clone(); + self.numer *= other.numer / gcd2.clone(); + self.denom /= gcd2; + self.denom *= other.denom / gcd1; + self.reduce(); //TODO: remove this line. see #8. } } @@ -575,15 +587,19 @@ mod opassign { impl DivAssign for Ratio { fn div_assign(&mut self, other: T) { - self.denom *= other; - self.reduce(); + let gcd = self.numer.gcd(&other); + self.numer /= gcd.clone(); + self.denom *= other / gcd; + self.reduce(); //TODO: remove this line. see #8. } } impl MulAssign for Ratio { fn mul_assign(&mut self, other: T) { - self.numer *= other; - self.reduce(); + let gcd = self.denom.gcd(&other); + self.denom /= gcd.clone(); + self.numer *= other / gcd; + self.reduce(); //TODO: remove this line. see #8. } } @@ -720,7 +736,12 @@ where type Output = Ratio; #[inline] fn mul(self, rhs: Ratio) -> Ratio { - Ratio::new(self.numer * rhs.numer, self.denom * rhs.denom) + let gcd1 = self.numer.gcd(&rhs.denom.clone()); + let gcd2 = self.denom.gcd(&rhs.numer.clone()); + Ratio::new( + self.numer / gcd1.clone() * (rhs.numer / gcd2.clone()), + self.denom / gcd2 * (rhs.denom / gcd1), + ) } } // a/b * c/1 = (a*c) / (b*1) = (a*c) / b @@ -731,7 +752,8 @@ where type Output = Ratio; #[inline] fn mul(self, rhs: T) -> Ratio { - Ratio::new(self.numer * rhs, self.denom) + let gcd = self.denom.gcd(&rhs); + Ratio::new(self.numer * (rhs / gcd.clone()), self.denom / gcd.clone()) } } @@ -745,7 +767,12 @@ where #[inline] fn div(self, rhs: Ratio) -> Ratio { - Ratio::new(self.numer * rhs.denom, self.denom * rhs.numer) + let numer_gcd = self.numer.gcd(&rhs.numer); + let denom_gcd = self.denom.gcd(&rhs.denom); + Ratio::new( + self.numer / numer_gcd.clone() * (rhs.denom / denom_gcd.clone()), + self.denom / denom_gcd * (rhs.numer / numer_gcd), + ) } } // (a/b) / (c/1) = (a*1) / (b*c) = a / (b*c) @@ -757,7 +784,8 @@ where #[inline] fn div(self, rhs: T) -> Ratio { - Ratio::new(self.numer, self.denom * rhs) + let gcd = self.numer.gcd(&rhs); + Ratio::new(self.numer / gcd.clone(), self.denom * (rhs / gcd)) } } @@ -1564,7 +1592,7 @@ mod test { use super::{to_big, _0, _1, _1_2, _2, _3_2, _5_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; - use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub}; + use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, NumAssign}; #[test] fn test_add() { @@ -1711,7 +1739,7 @@ mod test { test_sub_typed_overflow::(); test_sub_typed_overflow::(); #[cfg(has_u128)] - test_add_typed_overflow::(); + test_sub_typed_overflow::(); test_sub_typed_overflow::(); test_sub_typed_overflow::(); @@ -1756,6 +1784,52 @@ mod test { test_assign(_1_2, 2, _1); } + #[test] + fn test_mul_overflow() { + fn test_mul_typed_overflow() + where + T: Integer + Bounded + Clone + Debug + NumAssign, + { + // 1/big * 2/3 = 1/(max/4*3), where big is max/2 + let two = T::one() + T::one(); + let _3 = T::one() + T::one() + T::one(); + // make big = max/2, but also divisible by 2 + let big = T::max_value() / two.clone() / two.clone() * two.clone(); + let _1_big: Ratio = Ratio::new(T::one(), big.clone()); + let _2_3: Ratio = Ratio::new(two.clone(), _3.clone()); + let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); + assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); + let mut tmp = _1_big.clone(); + tmp *= _2_3; + assert_eq!(expected, tmp); + + // big/3 * 3 = big/1 + // make big = max/2, but make it indivisible by 3 + let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); + let big_3 = Ratio::new(big.clone(), _3.clone()); + let expected = Ratio::new(big.clone(), T::one()); + assert_eq!(expected, big_3.clone() * _3.clone()); + let mut tmp = big_3.clone(); + tmp *= _3.clone(); + assert_eq!(expected, tmp); + } + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + #[cfg(has_u128)] + test_mul_typed_overflow::(); + + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + test_mul_typed_overflow::(); + #[cfg(has_i128)] + test_mul_typed_overflow::(); + } + #[test] fn test_div() { fn test(a: Rational, b: Rational, c: Rational) { @@ -1790,6 +1864,52 @@ mod test { test_assign(_1, 2, _1_2); } + #[test] + fn test_div_overflow() { + fn test_div_typed_overflow() + where + T: Integer + Bounded + Clone + Debug + NumAssign, + { + // 1/big / 3/2 = 1/(max/4*3), where big is max/2 + let two = T::one() + T::one(); + let _3 = T::one() + T::one() + T::one(); + // big ~ max/2, and big is divisible by 2 + let big = T::max_value() / two.clone() / two.clone() * two.clone(); + let _1_big: Ratio = Ratio::new(T::one(), big.clone()); + let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); + let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); + assert_eq!(expected.clone(), _1_big.clone() / _3_two.clone()); + let mut tmp = _1_big.clone(); + tmp /= _3_two; + assert_eq!(expected, tmp); + + // 3/big / 3 = 1/big where big is max/2 + // big ~ max/2, and big is not divisible by 3 + let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); + let _3_big = Ratio::new(_3.clone(), big.clone()); + let expected = Ratio::new(T::one(), big.clone()); + assert_eq!(expected, _3_big.clone() / _3.clone()); + let mut tmp = _3_big.clone(); + tmp /= _3.clone(); + assert_eq!(expected, tmp); + } + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + #[cfg(has_u128)] + test_div_typed_overflow::(); + + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + test_div_typed_overflow::(); + #[cfg(has_i128)] + test_div_typed_overflow::(); + } + #[test] fn test_rem() { fn test(a: Rational, b: Rational, c: Rational) { From 692bc339f191dcdacc1a6ecc3a6ade4aeac781fb Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Sun, 16 Jun 2019 12:48:31 -0700 Subject: [PATCH 08/14] add overflow assertions in overflow tests add assertions that multiplying a number near the max value of its data type do indeed overflow. These assertions aim to guarantee that naive implementations of those operations would indeed overflow, in other words, that the generated large number is indeed large --- src/lib.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 791d623..3b95c20 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1592,6 +1592,7 @@ mod test { use super::{to_big, _0, _1, _1_2, _2, _3_2, _5_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; + use std::panic::{catch_unwind, AssertUnwindSafe}; use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, NumAssign}; #[test] @@ -1790,13 +1791,15 @@ mod test { where T: Integer + Bounded + Clone + Debug + NumAssign, { - // 1/big * 2/3 = 1/(max/4*3), where big is max/2 let two = T::one() + T::one(); let _3 = T::one() + T::one() + T::one(); + + // 1/big * 2/3 = 1/(max/4*3), where big is max/2 // make big = max/2, but also divisible by 2 let big = T::max_value() / two.clone() / two.clone() * two.clone(); let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _2_3: Ratio = Ratio::new(two.clone(), _3.clone()); + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); let mut tmp = _1_big.clone(); @@ -1806,6 +1809,7 @@ mod test { // big/3 * 3 = big/1 // make big = max/2, but make it indivisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); let big_3 = Ratio::new(big.clone(), _3.clone()); let expected = Ratio::new(big.clone(), T::one()); assert_eq!(expected, big_3.clone() * _3.clone()); @@ -1870,11 +1874,13 @@ mod test { where T: Integer + Bounded + Clone + Debug + NumAssign, { - // 1/big / 3/2 = 1/(max/4*3), where big is max/2 let two = T::one() + T::one(); let _3 = T::one() + T::one() + T::one(); + + // 1/big / 3/2 = 1/(max/4*3), where big is max/2 // big ~ max/2, and big is divisible by 2 let big = T::max_value() / two.clone() / two.clone() * two.clone(); + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); @@ -1886,6 +1892,7 @@ mod test { // 3/big / 3 = 1/big where big is max/2 // big ~ max/2, and big is not divisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); let _3_big = Ratio::new(_3.clone(), big.clone()); let expected = Ratio::new(T::one(), big.clone()); assert_eq!(expected, _3_big.clone() / _3.clone()); From c511749d796762740f09014a4d2e1940aeb30581 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Sun, 16 Jun 2019 17:48:55 -0700 Subject: [PATCH 09/14] make overflow tests always include Assign and non-Assign ops --- src/lib.rs | 92 +++++++++++++++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3b95c20..beb5937 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1637,36 +1637,18 @@ mod test { // Previously, this calculation would overflow. fn test_add_typed_overflow() where - T: Integer + Bounded + Clone + Debug + CheckedAdd + CheckedMul, + T: Integer + Bounded + Clone + Debug + NumAssign, { - let _6 = T::one() + T::one() + T::one() + T::one() + T::one() + T::one(); let _1_max = Ratio::new(T::one(), T::max_value()); let _2_max = Ratio::new(T::one() + T::one(), T::max_value()); - let _6_max = Ratio::new(_6, T::max_value()); assert_eq!(_1_max.clone() + _1_max.clone(), _2_max); - assert_eq!(_1_max.clone().checked_add(&_1_max), Some(_2_max.clone())); - assert_eq!( - _1_max.clone() - + _1_max.clone() - + _1_max.clone() - + _1_max.clone() - + _1_max.clone() - + _1_max.clone(), - _6_max - ); assert_eq!( - _1_max - .clone() - .checked_add(&_1_max) - .unwrap() - .checked_add(&_1_max) - .unwrap() - .checked_add(&_1_max) - .unwrap() - .checked_add(&_1_max) - .unwrap() - .checked_add(&_1_max), - Some(_6_max.clone()) + { + let mut tmp = _1_max.clone(); + tmp += _1_max.clone(); + tmp + }, + _2_max.clone() ); } test_add_typed_overflow::(); @@ -1726,13 +1708,15 @@ mod test { // for each integer type. Previously, this calculation would overflow. fn test_sub_typed_overflow() where - T: Integer + Bounded + Clone + Debug + CheckedSub + CheckedMul, + T: Integer + Bounded + Clone + Debug + NumAssign, { let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); assert!(T::is_zero(&(_1_max.clone() - _1_max.clone()).numer)); - assert!(T::is_zero( - &(_1_max.clone().checked_sub(&_1_max).unwrap().numer) - )); + { + let mut tmp: Ratio = _1_max.clone(); + tmp -= _1_max.clone(); + assert!(T::is_zero(&tmp.numer)); + } } test_sub_typed_overflow::(); test_sub_typed_overflow::(); @@ -1802,9 +1786,11 @@ mod test { assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); - let mut tmp = _1_big.clone(); - tmp *= _2_3; - assert_eq!(expected, tmp); + assert_eq!(expected, { + let mut tmp = _1_big.clone(); + tmp *= _2_3; + tmp + }); // big/3 * 3 = big/1 // make big = max/2, but make it indivisible by 3 @@ -1813,9 +1799,11 @@ mod test { let big_3 = Ratio::new(big.clone(), _3.clone()); let expected = Ratio::new(big.clone(), T::one()); assert_eq!(expected, big_3.clone() * _3.clone()); - let mut tmp = big_3.clone(); - tmp *= _3.clone(); - assert_eq!(expected, tmp); + assert_eq!(expected, { + let mut tmp = big_3.clone(); + tmp *= _3.clone(); + tmp + }); } test_mul_typed_overflow::(); test_mul_typed_overflow::(); @@ -1885,9 +1873,11 @@ mod test { let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() / _3_two.clone()); - let mut tmp = _1_big.clone(); - tmp /= _3_two; - assert_eq!(expected, tmp); + assert_eq!(expected, { + let mut tmp = _1_big.clone(); + tmp /= _3_two; + tmp + }); // 3/big / 3 = 1/big where big is max/2 // big ~ max/2, and big is not divisible by 3 @@ -1896,9 +1886,11 @@ mod test { let _3_big = Ratio::new(_3.clone(), big.clone()); let expected = Ratio::new(T::one(), big.clone()); assert_eq!(expected, _3_big.clone() / _3.clone()); - let mut tmp = _3_big.clone(); - tmp /= _3.clone(); - assert_eq!(expected, tmp); + assert_eq!(expected, { + let mut tmp = _3_big.clone(); + tmp /= _3.clone(); + tmp + }); } test_div_typed_overflow::(); test_div_typed_overflow::(); @@ -1953,15 +1945,23 @@ mod test { #[test] fn test_rem_overflow() { - // tests that Ratio(1,1) % Ratio(1, T::max_value()) equals 0 + // tests that Ratio(1,2) % Ratio(1, T::max_value()) equals 0 // for each integer type. Previously, this calculation would overflow. fn test_rem_typed_overflow() where - T: Integer + Bounded + Clone + Debug, + T: Integer + Bounded + Clone + Debug + NumAssign, { - let _1_max: Ratio = Ratio::new(T::one(), T::max_value()); - let _1_1: Ratio = Ratio::new(T::one(), T::one()); - assert!(T::is_zero(&(_1_1 % _1_max.clone()).numer)); + let two = T::one() + T::one(); + //value near to maximum, but divisible by two + let max_div2 = T::max_value() / two.clone() * two.clone(); + let _1_max: Ratio = Ratio::new(T::one(), max_div2.clone()); + let _1_two: Ratio = Ratio::new(T::one(), two); + assert!(T::is_zero(&(_1_two.clone() % _1_max.clone()).numer)); + { + let mut tmp: Ratio = _1_two.clone(); + tmp %= _1_max.clone(); + assert!(T::is_zero(&tmp.numer)); + } } test_rem_typed_overflow::(); test_rem_typed_overflow::(); From 1226b06919570f2c22049027834c5abdd43907b1 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Sun, 16 Jun 2019 18:10:28 -0700 Subject: [PATCH 10/14] remove dependency on std for overflow tests --- src/lib.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index beb5937..1d72600 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1592,6 +1592,7 @@ mod test { use super::{to_big, _0, _1, _1_2, _2, _3_2, _5_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; + #[cfg(feature = "std")] use std::panic::{catch_unwind, AssertUnwindSafe}; use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, NumAssign}; @@ -1783,7 +1784,9 @@ mod test { let big = T::max_value() / two.clone() / two.clone() * two.clone(); let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _2_3: Ratio = Ratio::new(two.clone(), _3.clone()); - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + if cfg!(std) { + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + } let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); assert_eq!(expected, { @@ -1795,7 +1798,9 @@ mod test { // big/3 * 3 = big/1 // make big = max/2, but make it indivisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + if cfg!(std) { + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + } let big_3 = Ratio::new(big.clone(), _3.clone()); let expected = Ratio::new(big.clone(), T::one()); assert_eq!(expected, big_3.clone() * _3.clone()); @@ -1868,7 +1873,9 @@ mod test { // 1/big / 3/2 = 1/(max/4*3), where big is max/2 // big ~ max/2, and big is divisible by 2 let big = T::max_value() / two.clone() / two.clone() * two.clone(); - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + if cfg!(std) { + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + } let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); @@ -1882,7 +1889,9 @@ mod test { // 3/big / 3 = 1/big where big is max/2 // big ~ max/2, and big is not divisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + if cfg!(std) { + assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); + } let _3_big = Ratio::new(_3.clone(), big.clone()); let expected = Ratio::new(T::one(), big.clone()); assert_eq!(expected, _3_big.clone() / _3.clone()); From 902051c541ce7eda53d5fc5a337e4ad05e2a829b Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Sun, 16 Jun 2019 19:14:48 -0700 Subject: [PATCH 11/14] replace *, catch_unwind with checked_mul --- src/lib.rs | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1d72600..55079c0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1592,8 +1592,6 @@ mod test { use super::{to_big, _0, _1, _1_2, _2, _3_2, _5_2, _NEG1_2}; use core::fmt::Debug; use integer::Integer; - #[cfg(feature = "std")] - use std::panic::{catch_unwind, AssertUnwindSafe}; use traits::{Bounded, CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, NumAssign}; #[test] @@ -1774,7 +1772,7 @@ mod test { fn test_mul_overflow() { fn test_mul_typed_overflow() where - T: Integer + Bounded + Clone + Debug + NumAssign, + T: Integer + Bounded + Clone + Debug + NumAssign + CheckedMul, { let two = T::one() + T::one(); let _3 = T::one() + T::one() + T::one(); @@ -1784,9 +1782,7 @@ mod test { let big = T::max_value() / two.clone() / two.clone() * two.clone(); let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _2_3: Ratio = Ratio::new(two.clone(), _3.clone()); - if cfg!(std) { - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); - } + assert_eq!(None, big.clone().checked_mul(&_3.clone())); let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); assert_eq!(expected, { @@ -1798,9 +1794,7 @@ mod test { // big/3 * 3 = big/1 // make big = max/2, but make it indivisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); - if cfg!(std) { - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); - } + assert_eq!(None, big.clone().checked_mul(&_3.clone())); let big_3 = Ratio::new(big.clone(), _3.clone()); let expected = Ratio::new(big.clone(), T::one()); assert_eq!(expected, big_3.clone() * _3.clone()); @@ -1865,7 +1859,7 @@ mod test { fn test_div_overflow() { fn test_div_typed_overflow() where - T: Integer + Bounded + Clone + Debug + NumAssign, + T: Integer + Bounded + Clone + Debug + NumAssign + CheckedMul, { let two = T::one() + T::one(); let _3 = T::one() + T::one() + T::one(); @@ -1873,9 +1867,7 @@ mod test { // 1/big / 3/2 = 1/(max/4*3), where big is max/2 // big ~ max/2, and big is divisible by 2 let big = T::max_value() / two.clone() / two.clone() * two.clone(); - if cfg!(std) { - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); - } + assert_eq!(None, big.clone().checked_mul(&_3.clone())); let _1_big: Ratio = Ratio::new(T::one(), big.clone()); let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); @@ -1889,9 +1881,7 @@ mod test { // 3/big / 3 = 1/big where big is max/2 // big ~ max/2, and big is not divisible by 3 let big = T::max_value() / two.clone() / _3.clone() * _3.clone() + T::one(); - if cfg!(std) { - assert!(catch_unwind(AssertUnwindSafe(|| big.clone() * _3.clone())).is_err()); - } + assert_eq!(None, big.clone().checked_mul(&_3.clone())); let _3_big = Ratio::new(_3.clone(), big.clone()); let expected = Ratio::new(T::one(), big.clone()); assert_eq!(expected, _3_big.clone() / _3.clone()); From 29031376fe66b6017d3e722724690c778215fc95 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Wed, 19 Jun 2019 07:36:29 -0700 Subject: [PATCH 12/14] improve variable naming, fix outdated comments in mul, div impls --- src/lib.rs | 50 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 55079c0..4309d6b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -519,30 +519,28 @@ mod opassign { } } - // (a/b) / (c/d) = (a/gcd1)*(d/gcd2) / ((c/gcd1)*(b/gcd2)) - // where gcd1 = gcd(a,c); gcd2 = gcd(b,d) + // (a/b) / (c/d) = (a/gcd_ac)*(d/gcd_bd) / ((c/gcd_ac)*(b/gcd_bd)) impl DivAssign for Ratio { fn div_assign(&mut self, other: Ratio) { - let numer_gcd = self.numer.gcd(&other.numer.clone()); - let denom_gcd = self.denom.gcd(&other.denom.clone()); - self.numer /= numer_gcd.clone(); - self.numer *= other.denom / denom_gcd.clone(); - self.denom /= denom_gcd.clone(); - self.denom *= other.numer / numer_gcd.clone(); + let gcd_ac = self.numer.gcd(&other.numer.clone()); + let gcd_bd = self.denom.gcd(&other.denom.clone()); + self.numer /= gcd_ac.clone(); + self.numer *= other.denom / gcd_bd.clone(); + self.denom /= gcd_bd.clone(); + self.denom *= other.numer / gcd_ac.clone(); self.reduce(); //TODO: remove this line. see #8. } } - // a/b * c/d = (a/gcd1)*(c/gcd2) / ((d/gcd1)*(b/gcd2)) - // where gcd1 = gcd(a,d); gcd2 = gcd(b,c) + // a/b * c/d = (a/gcd_ad)*(c/gcd_bc) / ((d/gcd_ad)*(b/gcd_bc)) impl MulAssign for Ratio { fn mul_assign(&mut self, other: Ratio) { - let gcd1 = self.numer.gcd(&other.denom.clone()); - let gcd2 = self.denom.gcd(&other.numer.clone()); - self.numer /= gcd1.clone(); - self.numer *= other.numer / gcd2.clone(); - self.denom /= gcd2; - self.denom *= other.denom / gcd1; + let gcd_ad = self.numer.gcd(&other.denom.clone()); + let gcd_bc = self.denom.gcd(&other.numer.clone()); + self.numer /= gcd_ad.clone(); + self.numer *= other.numer / gcd_bc.clone(); + self.denom /= gcd_bc; + self.denom *= other.denom / gcd_ad; self.reduce(); //TODO: remove this line. see #8. } } @@ -728,7 +726,7 @@ macro_rules! forward_all_binop { // Arithmetic forward_all_binop!(impl Mul, mul); -// a/b * c/d = (a*c)/(b*d) +// a/b * c/d = (a/gcd_ad)*(c/gcd_bc) / ((d/gcd_ad)*(b/gcd_bc)) impl Mul> for Ratio where T: Clone + Integer, @@ -736,11 +734,11 @@ where type Output = Ratio; #[inline] fn mul(self, rhs: Ratio) -> Ratio { - let gcd1 = self.numer.gcd(&rhs.denom.clone()); - let gcd2 = self.denom.gcd(&rhs.numer.clone()); + let gcd_ad = self.numer.gcd(&rhs.denom.clone()); + let gcd_bc = self.denom.gcd(&rhs.numer.clone()); Ratio::new( - self.numer / gcd1.clone() * (rhs.numer / gcd2.clone()), - self.denom / gcd2 * (rhs.denom / gcd1), + self.numer / gcd_ad.clone() * (rhs.numer / gcd_bc.clone()), + self.denom / gcd_bc * (rhs.denom / gcd_ad), ) } } @@ -758,7 +756,7 @@ where } forward_all_binop!(impl Div, div); -// (a/b) / (c/d) = (a*d) / (b*c) +// (a/b) / (c/d) = (a/gcd_ac)*(d/gcd_bd) / ((c/gcd_ac)*(b/gcd_bd)) impl Div> for Ratio where T: Clone + Integer, @@ -767,11 +765,11 @@ where #[inline] fn div(self, rhs: Ratio) -> Ratio { - let numer_gcd = self.numer.gcd(&rhs.numer); - let denom_gcd = self.denom.gcd(&rhs.denom); + let gcd_ac = self.numer.gcd(&rhs.numer); + let gcd_bd = self.denom.gcd(&rhs.denom); Ratio::new( - self.numer / numer_gcd.clone() * (rhs.denom / denom_gcd.clone()), - self.denom / denom_gcd * (rhs.numer / numer_gcd), + self.numer / gcd_ac.clone() * (rhs.denom / gcd_bd.clone()), + self.denom / gcd_bd * (rhs.numer / gcd_ac), ) } } From 6fab19d70399f4caacd119761b8d45dfd708a8a6 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Mon, 24 Jun 2019 13:36:44 -0700 Subject: [PATCH 13/14] fix #13 for checked_mul and checked_div --- src/lib.rs | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4309d6b..927b289 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -837,9 +837,12 @@ where { #[inline] fn checked_mul(&self, rhs: &Ratio) -> Option> { + let gcd_ad = self.numer.gcd(&rhs.denom); + let gcd_bc = self.denom.gcd(&rhs.numer); Some(Ratio::new( - otry!(self.numer.checked_mul(&rhs.numer)), - otry!(self.denom.checked_mul(&rhs.denom)), + otry!((self.numer.clone() / gcd_ad.clone()) + .checked_mul(&(rhs.numer.clone() / gcd_bc.clone()))), + otry!((self.denom.clone() / gcd_bc).checked_mul(&(rhs.denom.clone() / gcd_ad))), )) } } @@ -851,12 +854,17 @@ where { #[inline] fn checked_div(&self, rhs: &Ratio) -> Option> { - let bc = otry!(self.denom.checked_mul(&rhs.numer)); - if bc.is_zero() { - None - } else { - Some(Ratio::new(otry!(self.numer.checked_mul(&rhs.denom)), bc)) + let gcd_ac = self.numer.gcd(&rhs.numer); + let gcd_bd = self.denom.gcd(&rhs.denom); + let denom = otry!((self.denom.clone() / gcd_bd.clone()) + .checked_mul(&(rhs.numer.clone() / gcd_ac.clone()))); + if denom.is_zero() { + return None; } + Some(Ratio::new( + otry!((self.numer.clone() / gcd_ac).checked_mul(&(rhs.denom.clone() / gcd_bd))), + denom, + )) } } @@ -1783,6 +1791,10 @@ mod test { assert_eq!(None, big.clone().checked_mul(&_3.clone())); let expected = Ratio::new(T::one(), big / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() * _2_3.clone()); + assert_eq!( + Some(expected.clone()), + _1_big.clone().checked_mul(&_2_3.clone()) + ); assert_eq!(expected, { let mut tmp = _1_big.clone(); tmp *= _2_3; @@ -1870,6 +1882,10 @@ mod test { let _3_two: Ratio = Ratio::new(_3.clone(), two.clone()); let expected = Ratio::new(T::one(), big.clone() / two.clone() * _3.clone()); assert_eq!(expected.clone(), _1_big.clone() / _3_two.clone()); + assert_eq!( + Some(expected.clone()), + _1_big.clone().checked_div(&_3_two.clone()) + ); assert_eq!(expected, { let mut tmp = _1_big.clone(); tmp /= _3_two; From 7b83bb4966a79209c90f0396cdc580ab395ecb45 Mon Sep 17 00:00:00 2001 From: Max Blachman Date: Wed, 17 Jul 2019 17:49:11 -0700 Subject: [PATCH 14/14] remove extraneous clone()s when passing by reference --- src/lib.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 927b289..ea213ca 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -509,7 +509,7 @@ mod opassign { if self.denom == other.denom { self.numer += other.numer } else { - let lcm = self.denom.lcm(&other.denom.clone()); + let lcm = self.denom.lcm(&other.denom); let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); let rhs_numer = other.numer * (lcm.clone() / other.denom); self.numer = lhs_numer + rhs_numer; @@ -522,12 +522,12 @@ mod opassign { // (a/b) / (c/d) = (a/gcd_ac)*(d/gcd_bd) / ((c/gcd_ac)*(b/gcd_bd)) impl DivAssign for Ratio { fn div_assign(&mut self, other: Ratio) { - let gcd_ac = self.numer.gcd(&other.numer.clone()); - let gcd_bd = self.denom.gcd(&other.denom.clone()); + let gcd_ac = self.numer.gcd(&other.numer); + let gcd_bd = self.denom.gcd(&other.denom); self.numer /= gcd_ac.clone(); self.numer *= other.denom / gcd_bd.clone(); - self.denom /= gcd_bd.clone(); - self.denom *= other.numer / gcd_ac.clone(); + self.denom /= gcd_bd; + self.denom *= other.numer / gcd_ac; self.reduce(); //TODO: remove this line. see #8. } } @@ -535,8 +535,8 @@ mod opassign { // a/b * c/d = (a/gcd_ad)*(c/gcd_bc) / ((d/gcd_ad)*(b/gcd_bc)) impl MulAssign for Ratio { fn mul_assign(&mut self, other: Ratio) { - let gcd_ad = self.numer.gcd(&other.denom.clone()); - let gcd_bc = self.denom.gcd(&other.numer.clone()); + let gcd_ad = self.numer.gcd(&other.denom); + let gcd_bc = self.denom.gcd(&other.numer); self.numer /= gcd_ad.clone(); self.numer *= other.numer / gcd_bc.clone(); self.denom /= gcd_bc; @@ -550,7 +550,7 @@ mod opassign { if self.denom == other.denom { self.numer %= other.numer } else { - let lcm = self.denom.lcm(&other.denom.clone()); + let lcm = self.denom.lcm(&other.denom); let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); let rhs_numer = other.numer * (lcm.clone() / other.denom); self.numer = lhs_numer % rhs_numer; @@ -565,7 +565,7 @@ mod opassign { if self.denom == other.denom { self.numer -= other.numer } else { - let lcm = self.denom.lcm(&other.denom.clone()); + let lcm = self.denom.lcm(&other.denom); let lhs_numer = self.numer.clone() * (lcm.clone() / self.denom.clone()); let rhs_numer = other.numer * (lcm.clone() / other.denom); self.numer = lhs_numer - rhs_numer; @@ -734,8 +734,8 @@ where type Output = Ratio; #[inline] fn mul(self, rhs: Ratio) -> Ratio { - let gcd_ad = self.numer.gcd(&rhs.denom.clone()); - let gcd_bc = self.denom.gcd(&rhs.numer.clone()); + let gcd_ad = self.numer.gcd(&rhs.denom); + let gcd_bc = self.denom.gcd(&rhs.numer); Ratio::new( self.numer / gcd_ad.clone() * (rhs.numer / gcd_bc.clone()), self.denom / gcd_bc * (rhs.denom / gcd_ad), @@ -751,7 +751,7 @@ where #[inline] fn mul(self, rhs: T) -> Ratio { let gcd = self.denom.gcd(&rhs); - Ratio::new(self.numer * (rhs / gcd.clone()), self.denom / gcd.clone()) + Ratio::new(self.numer * (rhs / gcd.clone()), self.denom / gcd) } } @@ -798,7 +798,7 @@ macro_rules! arith_impl { if self.denom == rhs.denom { return Ratio::new(self.numer.$method(rhs.numer), rhs.denom); } - let lcm = self.denom.lcm(&rhs.denom.clone()); + let lcm = self.denom.lcm(&rhs.denom); let lhs_numer = self.numer * (lcm.clone() / self.denom); let rhs_numer = rhs.numer * (lcm.clone() / rhs.denom); Ratio::new(lhs_numer.$method(rhs_numer), lcm) @@ -874,7 +874,7 @@ macro_rules! checked_arith_impl { impl $imp for Ratio { #[inline] fn $method(&self, rhs: &Ratio) -> Option> { - let gcd = self.denom.clone().gcd(&rhs.denom.clone()); + let gcd = self.denom.clone().gcd(&rhs.denom); let lcm = otry!((self.denom.clone() / gcd.clone()).checked_mul(&rhs.denom)); let lhs_numer = otry!((lcm.clone() / self.denom.clone()).checked_mul(&self.numer)); let rhs_numer = otry!((lcm.clone() / rhs.denom.clone()).checked_mul(&rhs.numer));