Skip to content

Commit

Permalink
Revert PartialEq and PartialOrd with primitives
Browse files Browse the repository at this point in the history 
This manually reverts the new implementations from pull request #136. As
noted in issue #150, the mere existence of those impls can have a bad
effect on type inference in other parts of a crate, even from afar. All
comparisons of primitives with an unknown type become ambiguous whether
that's meant to compare with itself or a bigint, even if `num-bigint` is
not directly in scope at all.

Since this can break unrelated code in surprising ways, I think it's not
wise for us to have these implementations. Maybe we can explore other
methods to compare with primitives in the future, though it won't be as
convenient as using the operators.
  • Loading branch information
@cuviper
cuviper committed May 8, 2020
1 parent 821007a commit 7330b70
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 436 deletions.
15 changes: 0 additions & 15 deletions ci/big_quickcheck/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,18 +357,3 @@ fn quickcheck_modpow() {

qc.quickcheck(test_modpow as fn(i128, u128, i128) -> TestResult);
}

#[test]
fn quickcheck_scalar_cmp() {
let gen = StdThreadGen::new(usize::max_value());
let mut qc = QuickCheck::with_gen(gen);

fn test_cmp(lhs: i64, rhs: i64) -> TestResult {
let correct = lhs.partial_cmp(&rhs);
let big_lhs = BigInt::from(lhs);
let res = big_lhs.partial_cmp(&rhs);
TestResult::from_bool(correct == res)
}

qc.quickcheck(test_cmp as fn(i64, i64) -> TestResult);
}
35 changes: 0 additions & 35 deletions src/algorithms.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -811,16 +811,6 @@ fn biguint_shr2(n: Cow<'_, BigUint>, digits: usize, shift: u8) -> BigUint {
biguint_from_vec(data)
}

pub(crate) fn cmp_zero_padded_slice(mut a: &[BigDigit], mut b: &[BigDigit]) -> Ordering {
while let Some((&0, head)) = a.split_last() {
a = head;
}
while let Some((&0, head)) = b.split_last() {
b = head;
}
cmp_slice(a, b)
}

pub(crate) fn cmp_slice(a: &[BigDigit], b: &[BigDigit]) -> Ordering {
debug_assert!(a.last() != Some(&0));
debug_assert!(b.last() != Some(&0));
Expand Down Expand Up @@ -854,29 +844,4 @@ mod algorithm_tests {
assert_eq!(sub_sign_i(&a.data[..], &b.data[..]), &a_i - &b_i);
assert_eq!(sub_sign_i(&b.data[..], &a.data[..]), &b_i - &a_i);
}

#[test]
fn test_cmp_zero_padded_slice() {
use super::cmp_zero_padded_slice;
use core::cmp::Ordering::*;

assert_eq!(cmp_zero_padded_slice(&[1, 0], &[1]), Equal);
assert_eq!(cmp_zero_padded_slice(&[1], &[1, 0]), Equal);

assert_eq!(cmp_zero_padded_slice(&[0], &[0]), Equal);
assert_eq!(cmp_zero_padded_slice(&[], &[0]), Equal);
assert_eq!(cmp_zero_padded_slice(&[0], &[]), Equal);

assert_eq!(cmp_zero_padded_slice(&[1], &[0]), Greater);
assert_eq!(cmp_zero_padded_slice(&[1000], &[0]), Greater);
assert_eq!(cmp_zero_padded_slice(&[1000], &[0, 0, 0]), Greater);
assert_eq!(cmp_zero_padded_slice(&[1000, 0], &[0, 0, 0]), Greater);
assert_eq!(cmp_zero_padded_slice(&[0, 1000, 0], &[0, 1000]), Equal);

assert_eq!(cmp_zero_padded_slice(&[0], &[1]), Less);
assert_eq!(cmp_zero_padded_slice(&[0], &[1000]), Less);
assert_eq!(cmp_zero_padded_slice(&[0, 0, 0], &[1000]), Less);
assert_eq!(cmp_zero_padded_slice(&[0, 0, 0], &[1000, 0]), Less);
assert_eq!(cmp_zero_padded_slice(&[0, 1000], &[0, 1000, 0]), Equal);
}
}
20 changes: 0 additions & 20 deletions src/bigint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,26 +199,6 @@ impl PartialOrd for BigInt {
}
}

fn sign<T>(num: T) -> Sign
where
T: Ord + Zero,
{
if num < T::zero() {
Sign::Minus
} else if num > T::zero() {
Sign::Plus
} else {
Sign::NoSign
}
}

impl_partialord_partialeq_for_bigint!(i8, u8);
impl_partialord_partialeq_for_bigint!(i16, u16);
impl_partialord_partialeq_for_bigint!(i32, u32);
impl_partialord_partialeq_for_bigint!(i64, u64);
impl_partialord_partialeq_for_bigint!(i128, u128);
impl_partialord_partialeq_for_bigint!(isize, usize);

impl Ord for BigInt {
#[inline]
fn cmp(&self, other: &BigInt) -> Ordering {
Expand Down
50 changes: 1 addition & 49 deletions src/biguint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ mod monty;

use self::algorithms::{__add2, __sub2rev, add2, sub2, sub2rev};
use self::algorithms::{biguint_shl, biguint_shr};
use self::algorithms::{cmp_slice, cmp_zero_padded_slice, fls, ilog2};
use self::algorithms::{cmp_slice, fls, ilog2};
use self::algorithms::{div_rem, div_rem_digit, div_rem_ref, rem_digit};
use self::algorithms::{mac_with_carry, mul3, scalar_mul};
use self::monty::monty_modpow;
Expand Down Expand Up @@ -118,54 +118,6 @@ impl Ord for BigUint {
}
}

impl_partialord_partialeq_for_biguint_below_digit!(u8);
impl_partialord_partialeq_for_biguint_below_digit!(u16);
impl_partialord_partialeq_for_biguint_below_digit!(u32);

impl_scalar_partialeq!(impl PartialEq<u64> for BigUint);
impl_partialord_rev!(impl PartialOrd<u64> for BigUint);
impl PartialOrd<u64> for BigUint {
#[cfg(u64_digit)]
#[inline]
fn partial_cmp(&self, other: &u64) -> Option<Ordering> {
Some(cmp_zero_padded_slice(&self.data[..], &[*other]))
}

#[cfg(not(u64_digit))]
#[inline]
fn partial_cmp(&self, other: &u64) -> Option<Ordering> {
let (hi, lo) = big_digit::from_doublebigdigit(*other);
Some(cmp_zero_padded_slice(&self.data[..], &[lo, hi]))
}
}

impl_scalar_partialeq!(impl PartialEq<u128> for BigUint);
impl_partialord_rev!(impl PartialOrd<u128> for BigUint);
impl PartialOrd<u128> for BigUint {
#[cfg(u64_digit)]
#[inline]
fn partial_cmp(&self, other: &u128) -> Option<Ordering> {
let (hi, lo) = big_digit::from_doublebigdigit(*other);
Some(cmp_zero_padded_slice(&self.data[..], &[lo, hi]))
}

#[cfg(not(u64_digit))]
#[inline]
fn partial_cmp(&self, other: &u128) -> Option<Ordering> {
let (a, b, c, d) = u32_from_u128(*other);
Some(cmp_zero_padded_slice(&self.data[..], &[d, c, b, a]))
}
}

impl_scalar_partialeq!(impl PartialEq<usize> for BigUint);
impl_partialord_rev!(impl PartialOrd<usize> for BigUint);
impl PartialOrd<usize> for BigUint {
#[inline]
fn partial_cmp(&self, other: &usize) -> Option<Ordering> {
self.partial_cmp(&(*other as UsizePromotion))
}
}

impl Default for BigUint {
#[inline]
fn default() -> BigUint {
Expand Down
83 changes: 0 additions & 83 deletions src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,86 +439,3 @@ macro_rules! impl_product_iter_type {
}
};
}

/// Implements PartialEq based on PartialOrd
macro_rules! impl_scalar_partialeq {
(impl PartialEq < $scalar:ty > for $res:ty) => {
impl PartialEq<$scalar> for $res {
#[inline]
fn eq(&self, other: &$scalar) -> bool {
match self.partial_cmp(other) {
Some(Equal) => true,
_ => false,
}
}
}
impl PartialEq<$res> for $scalar {
#[inline]
fn eq(&self, other: &$res) -> bool {
match self.partial_cmp(other) {
Some(Equal) => true,
_ => false,
}
}
}
};
}

macro_rules! impl_partialord_rev {
(impl PartialOrd < $scalar:ty > for $typ:ty) => {
impl PartialOrd<$typ> for $scalar {
#[inline]
fn partial_cmp(&self, other: &$typ) -> Option<Ordering> {
other.partial_cmp(self).map(Ordering::reverse)
}
}
};
}

macro_rules! impl_partialord_partialeq_for_biguint_below_digit {
($typ_unsigned:ty) => {
impl_scalar_partialeq!(impl PartialEq<$typ_unsigned> for BigUint);
impl_partialord_rev!(impl PartialOrd<$typ_unsigned> for BigUint);
impl PartialOrd<$typ_unsigned> for BigUint {
#[inline]
fn partial_cmp(&self, other: &$typ_unsigned) -> Option<Ordering> {
Some(cmp_zero_padded_slice(&self.data[..], &[BigDigit::from(*other)]))
}
}
};
}

macro_rules! impl_partialord_partialeq_for_bigint {
($typ_signed:ty, $typ_unsigned:ty) => {
impl_scalar_partialeq!(impl PartialEq<$typ_signed> for BigInt);
impl_partialord_rev!(impl PartialOrd<$typ_signed> for BigInt);
impl PartialOrd<$typ_signed> for BigInt {
#[inline]
fn partial_cmp(&self, other: &$typ_signed) -> Option<Ordering> {
let scmp = self.sign().cmp(&sign(*other));
if scmp != Equal {
return Some(scmp);
}

let abs: $typ_unsigned = other.unsigned_abs();
match self.sign {
NoSign => Some(Equal),
Plus => self.data.partial_cmp(&abs),
Minus => abs.partial_cmp(&self.data),
}
}
}

impl_scalar_partialeq!(impl PartialEq<$typ_unsigned> for BigInt);
impl_partialord_rev!(impl PartialOrd<$typ_unsigned> for BigInt);
impl PartialOrd<$typ_unsigned> for BigInt {
#[inline]
fn partial_cmp(&self, other: &$typ_unsigned) -> Option<Ordering> {
match self.sign {
Minus => Some(Less),
_ => self.data.partial_cmp(other),
}
}
}
};
}
82 changes: 64 additions & 18 deletions tests/bigint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ fn test_from_bytes_be() {
check("AB", "16706");
check("Hello world!", "22405534230753963835153736737");
assert_eq!(BigInt::from_bytes_be(Plus, &[]), BigInt::zero());
assert_eq!(BigInt::from_bytes_be(Plus, &[]), 0);
assert_eq!(BigInt::from_bytes_be(Minus, &[]), BigInt::zero());
assert_eq!(BigInt::from_bytes_be(Minus, &[]), 0);
}

#[test]
Expand Down Expand Up @@ -1022,26 +1020,74 @@ fn test_lcm() {

#[test]
fn test_next_multiple_of() {
assert_eq!(BigInt::from(16).next_multiple_of(&BigInt::from(8)), 16);
assert_eq!(BigInt::from(23).next_multiple_of(&BigInt::from(8)), 24);
assert_eq!(BigInt::from(16).next_multiple_of(&BigInt::from(-8)), 16);
assert_eq!(BigInt::from(23).next_multiple_of(&BigInt::from(-8)), 16);
assert_eq!(BigInt::from(-16).next_multiple_of(&BigInt::from(8)), -16);
assert_eq!(BigInt::from(-23).next_multiple_of(&BigInt::from(8)), -16);
assert_eq!(BigInt::from(-16).next_multiple_of(&BigInt::from(-8)), -16);
assert_eq!(BigInt::from(-23).next_multiple_of(&BigInt::from(-8)), -24);
assert_eq!(
BigInt::from(16).next_multiple_of(&BigInt::from(8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(23).next_multiple_of(&BigInt::from(8)),
BigInt::from(24)
);
assert_eq!(
BigInt::from(16).next_multiple_of(&BigInt::from(-8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(23).next_multiple_of(&BigInt::from(-8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(-16).next_multiple_of(&BigInt::from(8)),
BigInt::from(-16)
);
assert_eq!(
BigInt::from(-23).next_multiple_of(&BigInt::from(8)),
BigInt::from(-16)
);
assert_eq!(
BigInt::from(-16).next_multiple_of(&BigInt::from(-8)),
BigInt::from(-16)
);
assert_eq!(
BigInt::from(-23).next_multiple_of(&BigInt::from(-8)),
BigInt::from(-24)
);
}

#[test]
fn test_prev_multiple_of() {
assert_eq!(BigInt::from(16).prev_multiple_of(&BigInt::from(8)), 16);
assert_eq!(BigInt::from(23).prev_multiple_of(&BigInt::from(8)), 16);
assert_eq!(BigInt::from(16).prev_multiple_of(&BigInt::from(-8)), 16);
assert_eq!(BigInt::from(23).prev_multiple_of(&BigInt::from(-8)), 24);
assert_eq!(BigInt::from(-16).prev_multiple_of(&BigInt::from(8)), -16);
assert_eq!(BigInt::from(-23).prev_multiple_of(&BigInt::from(8)), -24);
assert_eq!(BigInt::from(-16).prev_multiple_of(&BigInt::from(-8)), -16);
assert_eq!(BigInt::from(-23).prev_multiple_of(&BigInt::from(-8)), -16);
assert_eq!(
BigInt::from(16).prev_multiple_of(&BigInt::from(8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(23).prev_multiple_of(&BigInt::from(8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(16).prev_multiple_of(&BigInt::from(-8)),
BigInt::from(16)
);
assert_eq!(
BigInt::from(23).prev_multiple_of(&BigInt::from(-8)),
BigInt::from(24)
);
assert_eq!(
BigInt::from(-16).prev_multiple_of(&BigInt::from(8)),
BigInt::from(-16)
);
assert_eq!(
BigInt::from(-23).prev_multiple_of(&BigInt::from(8)),
BigInt::from(-24)
);
assert_eq!(
BigInt::from(-16).prev_multiple_of(&BigInt::from(-8)),
BigInt::from(-16)
);
assert_eq!(
BigInt::from(-23).prev_multiple_of(&BigInt::from(-8)),
BigInt::from(-16)
);
}

#[test]
Expand Down
Loading

0 comments on commit 7330b70

Please sign in to comment.