From 97128d2b9fa8faa05157b39dc48180d70babc11b Mon Sep 17 00:00:00 2001 From: Trevor Spiteri Date: Wed, 12 Feb 2020 17:32:34 +0100 Subject: [PATCH] fix checked_rem_int and fix+deprecate {wrapping,overflowing}_rem_int https://gitlab.com/tspiteri/fixed/issues/13 --- README.md | 3 ++ RELEASES.md | 3 ++ src/macros_frac.rs | 55 ++++++++++++++++++++ src/macros_no_frac.rs | 114 ------------------------------------------ src/traits.rs | 47 ++++++++--------- src/wrapping.rs | 39 ++++++++------- 6 files changed, 101 insertions(+), 160 deletions(-) diff --git a/README.md b/README.md index 27a6a61..2c94d11 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,8 @@ The conversions supported cover the following cases. ### Version 0.5.3 news (unreleased) + * Bug fix: all remainder operations with a fixed-point LHS and an + integer RHS were giving an incorrect answer ([issue 13]). * [`Rem`] and [`RemAssign`] were implemented for fixed-point numbers. * The following methods were added to all fixed-point types and to @@ -81,6 +83,7 @@ The conversions supported cover the following cases. * [`wrapping_div_euclid`] * [`overflowing_div_euclid`] +[issue 13]: https://gitlab.com/tspiteri/fixed/issues/13 [`RemAssign`]: https://doc.rust-lang.org/nightly/core/ops/trait.RemAssign.html [`Rem`]: https://doc.rust-lang.org/nightly/core/ops/trait.Rem.html [`checked_div_euclid`]: https://docs.rs/fixed/0.5.3/fixed/struct.FixedI32.html#method.checked_div_euclid diff --git a/RELEASES.md b/RELEASES.md index eaf005c..c149dbb 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -8,6 +8,9 @@ as-is, without any warranty. --> Version 0.5.3 (unreleased) ========================== + * Bug fix: all remainder operations with a fixed-point LHS and an + integer RHS were giving an incorrect answer + (https://gitlab.com/tspiteri/fixed/issues/13). * `Rem` and `RemAssign` were implemented for fixed-point numbers. * The following methods were added to all fixed-point types and to the `Fixed` trait: diff --git a/src/macros_frac.rs b/src/macros_frac.rs index 4d7622d..5fae7ac 100644 --- a/src/macros_frac.rs +++ b/src/macros_frac.rs @@ -265,6 +265,39 @@ assert_eq!(Fix::max_value().checked_div_euclid(Fix::from_num(0.25)), None); } } + comment! { + "Checked fixed-point remainder for division by an integer. +Returns the remainder, or [`None`] if the divisor is zero. + +# Examples + +```rust +use fixed::{types::extra::U4, ", $s_fixed, "}; +type Fix = ", $s_fixed, "; +assert_eq!(Fix::from_num(3.75).checked_rem_int(2), Some(Fix::from_num(1.75))); +assert_eq!(Fix::from_num(3.75).checked_rem_int(0), None); +", + if_signed_else_empty_str! { + $Signedness, + "assert_eq!(Fix::from_num(-3.75).checked_rem_int(2), Some(Fix::from_num(-1.75))); +", + }, + "``` + +[`None`]: https://doc.rust-lang.org/nightly/core/option/enum.Option.html#variant.None +"; + #[inline] + pub fn checked_rem_int(self, rhs: $Inner) -> Option<$Fixed> { + // Any overflow in coverting rhs to $Fixed means that |rhs| > |self|, + // and consequently the remainder is self. + let fixed_rhs = match Self::checked_from_num(rhs) { + Some(s) => s, + None => return Some(self), + }; + self.checked_rem(fixed_rhs) + } + } + comment! { "Saturating multiplication. Returns the product, saturating on overflow. @@ -537,6 +570,28 @@ assert_eq!(Fix::max_value().overflowing_div_euclid(Fix::from_num(0.25)), (wrappe (q, overflow) } } + + /// Remainder for division by an integer. + /// + /// # Panics + /// + /// Panics if the divisor is zero. + #[deprecated(since = "0.5.3", note = "cannot overflow, use `%` or `Rem::rem` instead")] + #[inline] + pub fn wrapping_rem_int(self, rhs: $Inner) -> $Fixed { + self % rhs + } + + /// Remainder for division by an integer. + /// + /// # Panics + /// + /// Panics if the divisor is zero. + #[deprecated(since = "0.5.3", note = "cannot overflow, use `%` or `Rem::rem` instead")] + #[inline] + pub fn overflowing_rem_int(self, rhs: $Inner) -> ($Fixed, bool) { + (self % rhs, false) + } } }; } diff --git a/src/macros_no_frac.rs b/src/macros_no_frac.rs index 691e8ef..a11682b 100644 --- a/src/macros_no_frac.rs +++ b/src/macros_no_frac.rs @@ -734,40 +734,6 @@ assert_eq!(Fix::from_num(1).checked_div_int(0), None); } } - comment! { - "Checked fixed-point remainder for division by an integer. -Returns the remainder, or [`None`] if the divisor is zero", - if_signed_unsigned! { - $Signedness, - " or if the division results in overflow.", - ".", - }, - " - -# Examples - -```rust -use fixed::{types::extra::U4, ", $s_fixed, "}; -type Fix = ", $s_fixed, "; -// binary 1.0101 / 8 = binary 0.0010 remainder 0.0101 -assert_eq!(Fix::from_bits(0b10101).checked_rem_int(8), Some(Fix::from_bits(0b101))); -assert_eq!(Fix::from_num(1).checked_rem_int(0), None); -", - if_signed_else_empty_str! { - $Signedness, - "assert_eq!(Fix::min_value().checked_rem_int(-1), None); -", - }, - "``` - -[`None`]: https://doc.rust-lang.org/nightly/core/option/enum.Option.html#variant.None -"; - #[inline] - pub fn checked_rem_int(self, rhs: $Inner) -> Option<$Fixed> { - self.to_bits().checked_rem(rhs).map(Self::from_bits) - } - } - comment! { "Checked remainder for Euclidean division. Returns the remainder, or [`None`] if the divisor is zero. @@ -1261,44 +1227,6 @@ assert_eq!(Fix::from_num(3).wrapping_div_int(2), one_point_5); } } - comment! { - "Wrapping fixed-point remainder for division by an integer. Returns the remainder", - if_signed_unsigned! { - $Signedness, - ", wrapping on overflow. - -Overflow can only occur when dividing the minimum value by −1.", - ". - -Can never overflow for unsigned values.", - }, - " - -# Panics - -Panics if the divisor is zero. - -# Examples - -```rust -use fixed::{types::extra::U4, ", $s_fixed, "}; -type Fix = ", $s_fixed, "; -// binary 1.0101 / 8 = binary 0.0010 remainder 0.0101 -assert_eq!(Fix::from_bits(0b10101).wrapping_rem_int(8), Fix::from_bits(0b101)); -", - if_signed_else_empty_str! { - $Signedness, - "assert_eq!(Fix::min_value().wrapping_rem_int(-1), 0); -", - }, - "``` -"; - #[inline] - pub fn wrapping_rem_int(self, rhs: $Inner) -> $Fixed { - Self::from_bits(self.to_bits().wrapping_rem(rhs)) - } - } - comment! { "Wrapping Euclidean division by an integer. Returns the quotient", if_signed_unsigned! { @@ -1611,48 +1539,6 @@ assert_eq!(Fix::from_num(3).overflowing_div_int(2), (one_point_5, false)); } } - comment! { - "Overflowing fixed-point remainder for division by an integer. - -Returns a [tuple] of the remainder and ", - if_signed_unsigned! { - $Signedness, - "a [`bool`] indicating whether an overflow has -occurred. On overflow, the wrapped value is returned. Overflow can -only occur when dividing the minimum value by −1.", - "[`false`][`bool`], as the division can never overflow for unsigned values.", - }, - " - -# Panics - -Panics if the divisor is zero. - -# Examples - -```rust -use fixed::{types::extra::U4, ", $s_fixed, "}; -type Fix = ", $s_fixed, "; -// binary 1.0101 / 8 = binary 0.0010 remainder 0.0101 -assert_eq!(Fix::from_bits(0b10101).overflowing_rem_int(8), (Fix::from_bits(0b101), false)); -", - if_signed_else_empty_str! { - $Signedness, - "assert_eq!(Fix::min_value().overflowing_rem_int(-1), (Fix::from_num(0), true)); -", - }, - "``` - -[`bool`]: https://doc.rust-lang.org/nightly/std/primitive.bool.html -[tuple]: https://doc.rust-lang.org/nightly/std/primitive.tuple.html -"; - #[inline] - pub fn overflowing_rem_int(self, rhs: $Inner) -> ($Fixed, bool) { - let (ans, o) = self.to_bits().overflowing_rem(rhs); - (Self::from_bits(ans), o) - } - } - comment! { "Overflowing Euclidean division by an integer. diff --git a/src/traits.rs b/src/traits.rs index f9aee63..22adde2 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -776,16 +776,6 @@ where /// Panics if the divisor is zero. fn wrapping_div_int(self, rhs: Self::Bits) -> Self; - /// Wrapping fixed-point remainder for division by an integer. - /// Returns the remainder, wrapping on overflow. - /// - /// Overflow can only occur when dividing the minimum value by −1. - /// - /// # Panics - /// - /// Panics if the divisor is zero. - fn wrapping_rem_int(self, rhs: Self::Bits) -> Self; - /// Wrapping Euclidean division by an integer. Returns the /// quotient, wrapping on overflow. /// @@ -906,21 +896,6 @@ where /// [tuple]: https://doc.rust-lang.org/nightly/std/primitive.tuple.html fn overflowing_div_int(self, rhs: Self::Bits) -> (Self, bool); - /// Overflowing fixed-point remainder for division by an integer. - /// - /// Returns a [tuple] of the remainder and a [`bool`], indicating - /// whether an overflow has occurred. On overflow, the wrapped - /// value is returned. Overflow can only occur when dividing the - /// minimum value by −1. - /// - /// # Panics - /// - /// Panics if the divisor is zero. - /// - /// [`bool`]: https://doc.rust-lang.org/nightly/std/primitive.bool.html - /// [tuple]: https://doc.rust-lang.org/nightly/std/primitive.tuple.html - fn overflowing_rem_int(self, rhs: Self::Bits) -> (Self, bool); - /// Overflowing Euclidean division by an integer. /// /// Returns a [tuple] of the quotient and a [`bool`], indicating @@ -969,6 +944,26 @@ where /// [`bool`]: https://doc.rust-lang.org/nightly/std/primitive.bool.html /// [tuple]: https://doc.rust-lang.org/nightly/std/primitive.tuple.html fn overflowing_shr(self, rhs: u32) -> (Self, bool); + + /// Remainder for division by an integer. + /// + /// # Panics + /// + /// Panics if the divisor is zero. + #[deprecated(since = "0.5.3", note = "cannot overflow, use `%` or `Rem::rem` instead")] + fn wrapping_rem_int(self, rhs: Self::Bits) -> Self { + self % rhs + } + + /// Remainder for division by an integer. + /// + /// # Panics + /// + /// Panics if the divisor is zero. + #[deprecated(since = "0.5.3", note = "cannot overflow, use `%` or `Rem::rem` instead")] + fn overflowing_rem_int(self, rhs: Self::Bits) -> (Self, bool) { + (self % rhs, false) + } } /// This trait provides methods common to all signed fixed-point numbers. @@ -1614,7 +1609,6 @@ macro_rules! impl_fixed { trait_delegate! { fn wrapping_div_euclid(self, rhs: Self) -> Self } trait_delegate! { fn wrapping_mul_int(self, rhs: Self::Bits) -> Self } trait_delegate! { fn wrapping_div_int(self, rhs: Self::Bits) -> Self } - trait_delegate! { fn wrapping_rem_int(self, rhs: Self::Bits) -> Self } trait_delegate! { fn wrapping_div_euclid_int(self, rhs: Self::Bits) -> Self } trait_delegate! { fn wrapping_rem_euclid_int(self, rhs: Self::Bits) -> Self } trait_delegate! { fn wrapping_shl(self, rhs: u32) -> Self } @@ -1627,7 +1621,6 @@ macro_rules! impl_fixed { trait_delegate! { fn overflowing_div_euclid(self, rhs: Self) -> (Self, bool) } trait_delegate! { fn overflowing_mul_int(self, rhs: Self::Bits) -> (Self, bool) } trait_delegate! { fn overflowing_div_int(self, rhs: Self::Bits) -> (Self, bool) } - trait_delegate! { fn overflowing_rem_int(self, rhs: Self::Bits) -> (Self, bool) } trait_delegate! { fn overflowing_div_euclid_int(self, rhs: Self::Bits) -> (Self, bool) } trait_delegate! { fn overflowing_rem_euclid_int(self, rhs: Self::Bits) -> (Self, bool) } trait_delegate! { fn overflowing_shl(self, rhs: u32) -> (Self, bool) } diff --git a/src/wrapping.rs b/src/wrapping.rs index 423eb6e..46c841a 100644 --- a/src/wrapping.rs +++ b/src/wrapping.rs @@ -20,6 +20,7 @@ use crate::{ traits::{Fixed, FixedSigned, FixedUnsigned, FromFixed, ToFixed}, FixedI128, FixedI16, FixedI32, FixedI64, FixedI8, FixedU128, FixedU16, FixedU32, FixedU64, FixedU8, + types::extra::{LeEqU128, LeEqU16, LeEqU32, LeEqU64, LeEqU8}, }; use core::{ fmt::{Display, Formatter, Result as FmtResult}, @@ -1016,45 +1017,45 @@ impl<'a, F: 'a + Fixed> Product<&'a Wrapping> for Wrapping { macro_rules! op_bits { ( - $Fixed:ident($Bits:ident)::$wrapping:ident, + $Fixed:ident($Bits:ident $(, $LeEqU:ident)*)::$wrapping:ident, $Op:ident $op:ident, $OpAssign:ident $op_assign:ident ) => { - impl $Op<$Bits> for Wrapping<$Fixed> { + impl $Op<$Bits> for Wrapping<$Fixed> { type Output = Wrapping<$Fixed>; #[inline] fn $op(self, other: $Bits) -> Wrapping<$Fixed> { Wrapping((self.0).$wrapping(other)) } } - impl<'a, Frac> $Op<$Bits> for &'a Wrapping<$Fixed> { + impl<'a, Frac $(: $LeEqU)*> $Op<$Bits> for &'a Wrapping<$Fixed> { type Output = Wrapping<$Fixed>; #[inline] fn $op(self, other: $Bits) -> Wrapping<$Fixed> { Wrapping((self.0).$wrapping(other)) } } - impl<'a, Frac> $Op<&'a $Bits> for Wrapping<$Fixed> { + impl<'a, Frac $(: $LeEqU)*> $Op<&'a $Bits> for Wrapping<$Fixed> { type Output = Wrapping<$Fixed>; #[inline] fn $op(self, other: &$Bits) -> Wrapping<$Fixed> { Wrapping((self.0).$wrapping(*other)) } } - impl<'a, 'b, Frac> $Op<&'a $Bits> for &'b Wrapping<$Fixed> { + impl<'a, 'b, Frac $(: $LeEqU)*> $Op<&'a $Bits> for &'b Wrapping<$Fixed> { type Output = Wrapping<$Fixed>; #[inline] fn $op(self, other: &$Bits) -> Wrapping<$Fixed> { Wrapping((self.0).$wrapping(*other)) } } - impl $OpAssign<$Bits> for Wrapping<$Fixed> { + impl $OpAssign<$Bits> for Wrapping<$Fixed> { #[inline] fn $op_assign(&mut self, other: $Bits) { self.0 = (self.0).$wrapping(other); } } - impl<'a, Frac> $OpAssign<&'a $Bits> for Wrapping<$Fixed> { + impl<'a, Frac $(: $LeEqU)*> $OpAssign<&'a $Bits> for Wrapping<$Fixed> { #[inline] fn $op_assign(&mut self, other: &$Bits) { self.0 = (self.0).$wrapping(*other); @@ -1064,19 +1065,19 @@ macro_rules! op_bits { } macro_rules! ops { - ($Fixed:ident($Bits:ident)) => { + ($Fixed:ident($Bits:ident, $LeEqU:ident)) => { op_bits! { $Fixed($Bits)::wrapping_mul_int, Mul mul, MulAssign mul_assign } op_bits! { $Fixed($Bits)::wrapping_div_int, Div div, DivAssign div_assign } - op_bits! { $Fixed($Bits)::wrapping_rem_int, Rem rem, RemAssign rem_assign } + op_bits! { $Fixed($Bits, $LeEqU)::rem, Rem rem, RemAssign rem_assign } }; } -ops! { FixedI8(i8) } -ops! { FixedI16(i16) } -ops! { FixedI32(i32) } -ops! { FixedI64(i64) } -ops! { FixedI128(i128) } -ops! { FixedU8(u8) } -ops! { FixedU16(u16) } -ops! { FixedU32(u32) } -ops! { FixedU64(u64) } -ops! { FixedU128(u128) } +ops! { FixedI8(i8, LeEqU8) } +ops! { FixedI16(i16, LeEqU16) } +ops! { FixedI32(i32, LeEqU32) } +ops! { FixedI64(i64, LeEqU64) } +ops! { FixedI128(i128, LeEqU128) } +ops! { FixedU8(u8, LeEqU8) } +ops! { FixedU16(u16, LeEqU16) } +ops! { FixedU32(u32, LeEqU32) } +ops! { FixedU64(u64, LeEqU64) } +ops! { FixedU128(u128, LeEqU128) }