Bug 796949 - Fix division and rounding of zero.

Fix division of 128-bit integers so that the remainder inherits the dividend's sign.
Fix rounding for truncated zero.

libgnucash/engine/gnc-int128.cpp

@@ -142,7 +142,7 @@ GncInt128::operator int64_t() const
 GncInt128::operator uint64_t() const
 {
     auto flags = get_flags(m_hi);
-    if (flags & neg)
+    if ((flags & neg) && !isZero()) // exclude negative zero
         throw std::underflow_error ("Can't represent negative value as uint64_t");
     if ((flags & (overflow | NaN)) || (m_hi || m_lo > UINT64_MAX))
         throw std::overflow_error ("Value to large to represent as uint64_t");
@@ -160,6 +160,7 @@ GncInt128::cmp (const GncInt128& b) const noexcept
         return 1;
     auto hi = get_num(m_hi);
     auto bhi = get_num(b.m_hi);
+    if (isZero() && b.isZero()) return 0;
     if (flags & neg)
     {
         if (!b.isNeg()) return -1;
@@ -478,6 +479,8 @@ GncInt128::operator*= (const GncInt128& b) noexcept
 {
     /* Test for 0 first */
     auto flags = get_flags(m_hi);
+    /* Handle the sign; ^ flips if b is negative */
+    flags ^= (get_flags(b.m_hi) & neg);
     if (isZero() || b.isZero())
     {
         m_lo = 0;
@@ -514,8 +517,7 @@ GncInt128::operator*= (const GncInt128& b) noexcept
         m_hi = set_flags(m_hi, flags);
         return *this;
     }
-    /* Handle the sign; ^ flips if b is negative */
+
-    flags ^= (get_flags(b.m_hi) & neg);
     /* The trivial case */
     if (abits + bbits <= legbits)
     {
@@ -602,6 +604,7 @@ div_multi_leg (uint64_t* u, size_t m, uint64_t* v, size_t n,
     uint64_t d {(UINT64_C(1) << sublegbits)/(v[n - 1] + UINT64_C(1))};
     uint64_t carry {UINT64_C(0)};
     bool negative {q.isNeg()};
+    bool rnegative {r.isNeg()};
     for (size_t i = 0; i < m; ++i)
     {
         u[i] = u[i] * d + carry;
@@ -689,6 +692,7 @@ div_multi_leg (uint64_t* u, size_t m, uint64_t* v, size_t n,
     r = GncInt128 ((u[3] << sublegbits) + u[2], (u[1] << sublegbits) + u[0]);
     r /= d;
     if (negative) q = -q;
+    if (rnegative) r = -r;
 }
 
 void
@@ -698,6 +702,7 @@ div_single_leg (uint64_t* u, size_t m, uint64_t v,
     uint64_t qv[sublegs] {};
     uint64_t carry {};
     bool negative {q.isNeg()};
+    bool rnegative {r.isNeg()};
     for (int i = m - 1; i >= 0; --i)
     {
         qv[i] = u[i] / v;
@@ -713,13 +718,18 @@ div_single_leg (uint64_t* u, size_t m, uint64_t v,
     q = GncInt128 ((qv[3] << sublegbits) + qv[2], (qv[1] << sublegbits) + qv[0]);
     r = GncInt128 ((u[3] << sublegbits) + u[2], (u[1] << sublegbits) + u[0]);
     if (negative) q = -q;
+    if (rnegative) r = -r;
 }
 
 }// namespace
 
- void
+void
 GncInt128::div (const GncInt128& b, GncInt128& q, GncInt128& r) const noexcept
 {
+    // clear remainder and quotient
+    r = GncInt128(0);
+    q = GncInt128(0);
+
     auto qflags = get_flags(q.m_hi);
     auto rflags = get_flags(r.m_hi);
     if (isOverflow() || b.isOverflow())
@@ -745,6 +755,7 @@ GncInt128::div (const GncInt128& b, GncInt128& q, GncInt128& r) const noexcept
     assert (&r != &b);
 
     q.zero(), r.zero();
+    qflags = rflags = 0;
     if (b.isZero())
     {
         qflags |= NaN;
@@ -755,11 +766,17 @@ GncInt128::div (const GncInt128& b, GncInt128& q, GncInt128& r) const noexcept
     }
 
     if (isNeg())
+    {
         qflags |= neg;
+        rflags |= neg;          // the remainder inherits the dividend's sign
+    }
 
     if (b.isNeg())
         qflags ^= neg;
 
+    q.m_hi = set_flags(q.m_hi, qflags);
+    r.m_hi = set_flags(r.m_hi, rflags);
+
     if (abs() < b.abs())
     {
         r = *this;
@@ -767,7 +784,7 @@ GncInt128::div (const GncInt128& b, GncInt128& q, GncInt128& r) const noexcept
     }
     auto hi = get_num(m_hi);
     auto bhi = get_num(b.m_hi);
-    q.m_hi = set_flags(hi, qflags);
+
     if (hi == 0 && bhi == 0) //let the hardware do it
     {
         assert (b.m_lo != 0); // b.m_hi is 0 but b isn't or we didn't get here.
@@ -914,6 +931,11 @@ GncInt128::asCharBufR(char* buf) const noexcept
         sprintf (buf, "%s", "NaN");
         return buf;
     }
+    if (isZero())
+    {
+        sprintf (buf, "%d", 0);
+        return buf;
+    }
     uint64_t d[dec_array_size] {};
     decimal_from_binary(d, get_num(m_hi), m_lo);
     char* next = buf;

libgnucash/engine/gnc-rational-rounding.hpp

@@ -25,6 +25,12 @@
 #include "gnc-numeric.h"
 #include "gnc-int128.hpp"
 
+template <typename T> inline bool
+quotient_is_positive(T dividend, T divisor)
+{
+    return (dividend > 0 && divisor > 0) || (dividend < 0 && divisor < 0);
+}
+
 enum class RoundType
 {
     floor = GNC_HOW_RND_FLOOR,
@@ -72,8 +78,23 @@ round(T num, T den, T rem, RT2T<RoundType::floor>)
 //              << ", and rem " << rem << ".\n";
     if (rem == 0)
         return num;
-    if (num < 0)
+    // floor num==0 that is the quotient of two numbers with opposite signs
-        return num + 1;
+    if (num < 0 || (num == 0 && !quotient_is_positive(rem, den)))
+        return num - 1;
+    return num;
+}
+
+
+template <> inline GncInt128
+round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
+                 RT2T<RoundType::floor>)
+{
+//    std::cout << "Rounding to floor  with num " << num << " den " << den
+//              << ", and rem " << rem << ".\n";
+    if (rem == 0)
+        return num;
+    if (num.isNeg())
+        return num - 1;
     return num;
 }
 
@@ -82,7 +103,18 @@ round(T num, T den, T rem, RT2T<RoundType::ceiling>)
 {
     if (rem == 0)
         return num;
-    if (num > 0)
+    if (num > 0 || (num == 0 && quotient_is_positive(rem, den)))
+        return num + 1;
+    return num;
+}
+
+template <> inline GncInt128
+round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
+      RT2T<RoundType::ceiling>)
+{
+    if (rem == 0)
+        return num;
+    if (!num.isNeg())
         return num + 1;
     return num;
 }
@@ -98,16 +130,31 @@ round(T num, T den, T rem, RT2T<RoundType::promote>)
 {
     if (rem == 0)
         return num;
+    if (num == 0)
+        return (!quotient_is_positive(rem, den) ? -1 : 1);
     return num + (num < 0 ? -1 : 1);
 }
 
+template <> inline GncInt128
+round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
+                 RT2T<RoundType::promote>)
+{
+    if (rem == 0)
+        return num;
+    return num + (num.isNeg() ? -1 : 1);
+}
+
 template <typename T> inline T
 round(T num, T den, T rem, RT2T<RoundType::half_down>)
 {
     if (rem == 0)
         return num;
     if (std::abs(rem * 2) > std::abs(den))
+    {
+        if (num == 0)
+            return (!quotient_is_positive(rem, den) ? -1 : 1);
         return num + (num < 0 ? -1 : 1);
+    }
     return num;
 }
 
@@ -118,7 +165,7 @@ round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
     if (rem == 0)
         return num;
     if (rem.abs() * 2 > den.abs())
-        return num + (num < 0 ? -1 : 1);
+        return num + (num.isNeg() ? -1 : 1);
     return num;
 }
 
@@ -128,7 +175,11 @@ round(T num, T den, T rem, RT2T<RoundType::half_up>)
     if (rem == 0)
         return num;
     if (std::abs(rem) * 2 >= std::abs(den))
+    {
+        if (num == 0)
+            return (!quotient_is_positive(rem, den) ? -1 : 1);
         return num + (num < 0 ? -1 : 1);
+    }
     return num;
 }
 
@@ -139,7 +190,7 @@ round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
     if (rem == 0)
         return num;
     if (rem.abs() * 2 >= den.abs())
-        return num + (num < 0 ? -1 : 1);
+        return num + (num.isNeg() ? -1 : 1);
     return num;
 }
 
@@ -150,11 +201,15 @@ round(T num, T den, T rem, RT2T<RoundType::bankers>)
         return num;
     if (std::abs(rem * 2) > std::abs(den) ||
         (std::abs(rem * 2) == std::abs(den) && num % 2))
-        return num += (num < 0 ? -1 : 1);
+    {
+        if (num == 0)
+            return (!quotient_is_positive(rem, den) ? -1 : 1);
+        return num + (num < 0 ? -1 : 1);
+    }
     return num;
 }
 
-template<> inline GncInt128
+template <> inline GncInt128
 round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
                  RT2T<RoundType::bankers>)
 {
@@ -162,7 +217,7 @@ round<GncInt128>(GncInt128 num, GncInt128 den, GncInt128 rem,
         return num;
     if (rem.abs() * 2 > den.abs() ||
         (rem.abs() * 2 == den.abs() && num % 2))
-        return num += (num < 0 ? -1 : 1);
+        return num + (num.isNeg() ? -1 : 1);
     return num;
 }
 #endif //__GNC_RATIONAL_ROUNDING_HPP__

libgnucash/engine/test/gtest-gnc-int128.cpp

@@ -432,7 +432,7 @@ TEST(GncInt128_functions, divide)
     GncInt128 big (sarg, barg);
     GncInt128 bigger (static_cast<uint64_t>(sarg), uarg);
     GncInt128 nsmall = -small;
-    GncInt128 nbig = -bigger;
+    GncInt128 nbigger = -bigger;
 
     EXPECT_EQ (GncInt128(INT64_C(0)), zero /= smallest);
     EXPECT_EQ (GncInt128(INT64_C(0)), zero %= smallest);
@@ -451,11 +451,20 @@ TEST(GncInt128_functions, divide)
 
     nsmall.div (smaller, q, r);
     EXPECT_EQ (-two, q);
-    EXPECT_EQ (GncInt128(INT64_C(3810195028972355394)), r);
+    EXPECT_EQ (GncInt128(INT64_C(-3810195028972355394)), r);
 
     nsmall.div (-smaller, q, r);
     EXPECT_EQ (two, q);
-    EXPECT_EQ (GncInt128(INT64_C(3810195028972355394)), r);
+    EXPECT_EQ (GncInt128(INT64_C(-3810195028972355394)), r);
+
+    smaller.div (small, q, r);
+    EXPECT_EQ (zero, q);
+    EXPECT_EQ (smaller, r);
+
+    smaller.div (nsmall, q, r);
+    EXPECT_EQ (zero, q);
+    EXPECT_TRUE (q.isNeg());
+    EXPECT_EQ (smaller, r);
 
     bigger.div (bigger, q, r);
     EXPECT_EQ (one, q);
@@ -477,18 +486,23 @@ TEST(GncInt128_functions, divide)
     EXPECT_EQ (-two, q);
     EXPECT_EQ (GncInt128(UINT64_C(3810195028972355394)), r);
 
-    nbig.div (-big, q, r);
+    nbigger.div (-big, q, r);
     EXPECT_EQ (two, q);
-    EXPECT_EQ (GncInt128(UINT64_C(3810195028972355394)), r);
+    EXPECT_EQ (GncInt128(0, UINT64_C(3810195028972355394), GncInt128::neg), r);
 
-    nbig.div (-big, q, r);
+    nbigger.div (-big, q, r);
     EXPECT_EQ (two, q);
-    EXPECT_EQ (GncInt128(UINT64_C(3810195028972355394)), r);
+    EXPECT_EQ (GncInt128(0, UINT64_C(3810195028972355394), GncInt128::neg), r);
 
     big.div (bigger, q, r);
     EXPECT_EQ (zero, q);
     EXPECT_EQ (big, r);
 
+    big.div (nbigger, q, r);
+    EXPECT_EQ (zero, q);
+    EXPECT_TRUE (q.isNeg());
+    EXPECT_EQ (big, r);
+
     big.div (big - 1, q, r);
     EXPECT_EQ(one, q);
     EXPECT_EQ(one, r);

libgnucash/engine/test/gtest-gnc-numeric.cpp

@@ -411,7 +411,7 @@ TEST(gncnumeric_functions, test_convert)
     EXPECT_EQ(3465453, c.num());
     EXPECT_EQ(128, c.denom());
     ASSERT_NO_THROW(c = b.convert<RoundType::floor>(128));
-    EXPECT_EQ(-3465452, c.num());
+    EXPECT_EQ(-3465454, c.num());
     EXPECT_EQ(128, c.denom());
     ASSERT_NO_THROW(c = a.convert<RoundType::ceiling>(128));
     EXPECT_EQ(3465454, c.num());