Fix bug: unchecked sub/add/mul cause negative zero and cause compare …

…error
yashan-technologies · Oct 26, 2022 · fb52a0d · fb52a0d
1 parent ba4cc71
commit fb52a0d
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 5 deletions.
diff --git a/benches/benchmark.rs b/benches/benchmark.rs
@@ -15,7 +15,7 @@
 //! decimal-rs benchmark
 
 use bencher::{benchmark_group, benchmark_main, black_box, Bencher};
-use decimal_rs::{Decimal, DecimalConvertError, MAX_BINARY_SIZE};
+use decimal_rs::{Decimal, DecimalConvertError, DECIMAL128, MAX_BINARY_SIZE};
 use std::collections::hash_map::DefaultHasher;
 use std::convert::{TryFrom, TryInto};
 use std::hash::Hash;
@@ -286,6 +286,80 @@ fn decimal_floor_100_times(bench: &mut Bencher) {
     })
 }
 
+#[inline(always)]
+fn add_with_same_scale(x: &Decimal, y: &Decimal) -> Decimal {
+    unsafe { x.add_with_same_scale_unchecked::<DECIMAL128>(y, 8) }
+}
+
+fn decimal_uncheck_add_same_scale_100_times(bench: &mut Bencher) {
+    let x = parse("901.23456789");
+    let y = parse("8901.23456789");
+    bench.iter(|| {
+        for _ in 0..100 {
+            let _n = add_with_same_scale(black_box(&x), black_box(&y));
+        }
+    })
+}
+
+#[inline(always)]
+fn add_with_same_scale_negative(x: &Decimal, y: &Decimal) -> Decimal {
+    unsafe { x.add_with_same_scale_and_negative_unchecked::<DECIMAL128>(y, 8, true) }
+}
+
+fn decimal_uncheck_add_same_scale_negative_100_times(bench: &mut Bencher) {
+    let x = parse("1891.23456789");
+    let y = parse("6701.23456789");
+    bench.iter(|| {
+        for _ in 0..100 {
+            let _n = add_with_same_scale_negative(black_box(&x), black_box(&y));
+        }
+    })
+}
+
+#[inline(always)]
+fn sub_with_same_scale(x: &Decimal, y: &Decimal) -> Decimal {
+    unsafe { x.sub_with_same_scale_unchecked::<DECIMAL128>(y, 8) }
+}
+
+fn decimal_uncheck_sub_100_times(bench: &mut Bencher) {
+    let x = parse("11.23456789");
+    let y = parse("71.23456789");
+    bench.iter(|| {
+        for _ in 0..100 {
+            let _n = sub_with_same_scale(black_box(&x), black_box(&y));
+        }
+    })
+}
+
+#[inline(always)]
+fn mull_unchecked(x: &Decimal, y: &Decimal) -> Decimal {
+    unsafe { x.mul_unchecked::<DECIMAL128>(y, 16) }
+}
+
+fn decimal_uncheck_mul_100_times(bench: &mut Bencher) {
+    let x = parse("1901.23456789");
+    let y = parse("7901.23456789");
+    bench.iter(|| {
+        for _ in 0..100 {
+            let _n = mull_unchecked(black_box(&x), black_box(&y));
+        }
+    })
+}
+
+#[inline(always)]
+fn cmp_zero(x: i128) -> bool {
+    x != 0
+}
+
+fn i128_cmp_zero_100_times(bench: &mut Bencher) {
+    let x = 12345678901;
+    bench.iter(|| {
+        for _ in 0..100 {
+            let _n = cmp_zero(black_box(x));
+        }
+    })
+}
+
 benchmark_group!(
     decimal_benches,
     decimal_parse,
@@ -317,6 +391,11 @@ benchmark_group!(
     decimal_exp,
     decimal_ceil_100_times,
     decimal_floor_100_times,
+    decimal_uncheck_add_same_scale_100_times,
+    decimal_uncheck_add_same_scale_negative_100_times,
+    decimal_uncheck_sub_100_times,
+    decimal_uncheck_mul_100_times,
+    i128_cmp_zero_100_times
 );
 
 benchmark_main!(decimal_benches);
diff --git a/src/convert.rs b/src/convert.rs
@@ -71,7 +71,7 @@ impl TryFrom<i128> for Decimal {
 
     #[inline]
     fn try_from(val: i128) -> std::result::Result<Self, Self::Error> {
-        if val > MAX_I128_REPR || val < -MAX_I128_REPR {
+        if !(-MAX_I128_REPR..=MAX_I128_REPR).contains(&val) {
             Err(DecimalConvertError::Overflow)
         } else {
             let (int_val, negative) = if val < 0 {

diff --git a/src/decimal.rs b/src/decimal.rs
@@ -943,7 +943,7 @@ impl Decimal {
             _ => self.int_val + other.int_val,
         };
 
-        Decimal::from_raw_parts(val, scale, negative)
+        Decimal::from_parts_unchecked(val, scale, negative)
     }
 
     #[inline]
@@ -1031,7 +1031,7 @@ impl Decimal {
                 }
             }
         };
-        Decimal::from_raw_parts(val, scale, neg)
+        Decimal::from_parts_unchecked(val, scale, neg)
     }
 
     /// Add two decimals.
@@ -1151,7 +1151,7 @@ impl Decimal {
             DECIMAL64 => ((self.int_val) as u64 * (other.int_val as u64)) as u128,
             _ => self.int_val * other.int_val,
         };
-        Decimal::from_raw_parts(val, scale, negative)
+        Decimal::from_parts_unchecked(val, scale, negative)
     }
 
     /// Checked decimal division.
@@ -3560,4 +3560,39 @@ mod tests {
             12,
         );
     }
+
+    #[test]
+    fn test_unchecked_and_compare() {
+        unsafe {
+            let left = Decimal::from_raw_parts(123, 2, true);
+            let right = Decimal::from_raw_parts(0, 1, false);
+            let mul_val = left.mul_unchecked::<DECIMAL128>(&right, 3);
+            let expect = Decimal::from_raw_parts(0, 45, false);
+            assert_eq!(mul_val.cmp(&expect), Ordering::Equal);
+        }
+
+        unsafe {
+            let left = Decimal::from_raw_parts(0, 2, true);
+            let right = Decimal::from_raw_parts(0, 2, true);
+            let val = left.sub_with_same_scale_unchecked::<DECIMAL128>(&right, 2);
+            let expect = Decimal::from_raw_parts(0, 45, false);
+            assert_eq!(val.cmp(&expect), Ordering::Equal);
+        }
+
+        unsafe {
+            let left = Decimal::from_raw_parts(0, 2, true);
+            let right = Decimal::from_raw_parts(0, 2, true);
+            let val = left.add_with_same_scale_unchecked::<DECIMAL128>(&right, 2);
+            let expect = Decimal::from_raw_parts(0, 45, false);
+            assert_eq!(val.cmp(&expect), Ordering::Equal);
+        }
+
+        unsafe {
+            let left = Decimal::from_raw_parts(0, 2, true);
+            let right = Decimal::from_raw_parts(0, 2, true);
+            let val = left.add_with_same_scale_and_negative_unchecked::<DECIMAL128>(&right, 2, true);
+            let expect = Decimal::from_raw_parts(0, 45, false);
+            assert_eq!(val.cmp(&expect), Ordering::Equal);
+        }
+    }
 }