From aab89065da63db13d003dc75a1f63a2840928667 Mon Sep 17 00:00:00 2001 From: David Palma Date: Sat, 4 May 2019 15:02:02 +0100 Subject: [PATCH] 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 | 56 ++++++++++----- libgnucash/engine/gnc-rational-rounding.hpp | 71 +++++++++++++++++--- libgnucash/engine/test/gtest-gnc-int128.cpp | 28 ++++++-- libgnucash/engine/test/gtest-gnc-numeric.cpp | 2 +- 4 files changed, 124 insertions(+), 33 deletions(-) diff --git a/libgnucash/engine/gnc-int128.cpp b/libgnucash/engine/gnc-int128.cpp index e545649f72..8ec77f5ce2 100644 --- a/libgnucash/engine/gnc-int128.cpp +++ b/libgnucash/engine/gnc-int128.cpp @@ -71,7 +71,7 @@ GncInt128::GncInt128 (int64_t upper, int64_t lower, uint8_t flags) : m_lo {static_cast(lower < 0 ? -lower : lower)} { if ((upper < 0 && lower > 0) || (upper > 0 && lower < 0)) - m_lo = (m_hi << 63) - m_lo; + m_lo = (m_hi << 63) - m_lo; else m_lo += (m_hi << 63); @@ -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,14 +160,15 @@ 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; - if (hi > bhi) return -1; - if (hi < bhi) return 1; - if (m_lo > b.m_lo) return -1; - if (m_lo < b.m_lo) return 1; - return 0; + if (!b.isNeg()) return -1; + if (hi > bhi) return -1; + if (hi < bhi) return 1; + if (m_lo > b.m_lo) return -1; + if (m_lo < b.m_lo) return 1; + return 0; } if (b.isNeg()) return 1; if (hi < bhi) return -1; @@ -309,9 +310,9 @@ GncInt128::operator-() const noexcept auto retval = *this; auto flags = get_flags(retval.m_hi); if (isNeg()) - flags ^= neg; + flags ^= neg; else - flags |= neg; + flags |= neg; retval.m_hi = set_flags(retval.m_hi, flags); return retval; } @@ -357,7 +358,7 @@ GncInt128::operator+= (const GncInt128& b) noexcept if (isOverflow() || isNan()) return *this; if ((isNeg () && !b.isNeg ()) || (!isNeg () && b.isNeg ())) - return this->operator-= (-b); + return this->operator-= (-b); uint64_t result = m_lo + b.m_lo; uint64_t carry = static_cast(result < m_lo); //Wrapping m_lo = result; @@ -365,7 +366,7 @@ GncInt128::operator+= (const GncInt128& b) noexcept auto bhi = get_num(b.m_hi); result = hi + bhi + carry; if (result < hi || result & flagmask) - flags |= overflow; + flags |= overflow; m_hi = set_flags(result, flags); return *this; } @@ -439,7 +440,7 @@ GncInt128::operator-= (const GncInt128& b) noexcept return *this; if ((!isNeg() && b.isNeg()) || (isNeg() && !b.isNeg())) - return this->operator+= (-b); + return this->operator+= (-b); bool operand_bigger {abs().cmp (b.abs()) < 0}; auto hi = get_num(m_hi); auto far_hi = get_num(b.m_hi); @@ -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; diff --git a/libgnucash/engine/gnc-rational-rounding.hpp b/libgnucash/engine/gnc-rational-rounding.hpp index 692301701e..11987b6cb9 100644 --- a/libgnucash/engine/gnc-rational-rounding.hpp +++ b/libgnucash/engine/gnc-rational-rounding.hpp @@ -25,6 +25,12 @@ #include "gnc-numeric.h" #include "gnc-int128.hpp" +template 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) // << ", and rem " << rem << ".\n"; if (rem == 0) return num; - if (num < 0) - return num + 1; + // floor num==0 that is the quotient of two numbers with opposite signs + if (num < 0 || (num == 0 && !quotient_is_positive(rem, den))) + return num - 1; + return num; +} + + +template <> inline GncInt128 +round(GncInt128 num, GncInt128 den, GncInt128 rem, + RT2T) +{ +// 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) { 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 num, GncInt128 den, GncInt128 rem, + RT2T) +{ + 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) { 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 num, GncInt128 den, GncInt128 rem, + RT2T) +{ + if (rem == 0) + return num; + return num + (num.isNeg() ? -1 : 1); +} + template inline T round(T num, T den, T rem, RT2T) { 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 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) 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 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) 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 num, GncInt128 den, GncInt128 rem, RT2T) { @@ -162,7 +217,7 @@ round(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__ diff --git a/libgnucash/engine/test/gtest-gnc-int128.cpp b/libgnucash/engine/test/gtest-gnc-int128.cpp index 5aa04f8f6d..82ed98ba03 100644 --- a/libgnucash/engine/test/gtest-gnc-int128.cpp +++ b/libgnucash/engine/test/gtest-gnc-int128.cpp @@ -432,7 +432,7 @@ TEST(GncInt128_functions, divide) GncInt128 big (sarg, barg); GncInt128 bigger (static_cast(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); diff --git a/libgnucash/engine/test/gtest-gnc-numeric.cpp b/libgnucash/engine/test/gtest-gnc-numeric.cpp index f6ba3f114b..680b6d82d9 100644 --- a/libgnucash/engine/test/gtest-gnc-numeric.cpp +++ b/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(128)); - EXPECT_EQ(-3465452, c.num()); + EXPECT_EQ(-3465454, c.num()); EXPECT_EQ(128, c.denom()); ASSERT_NO_THROW(c = a.convert(128)); EXPECT_EQ(3465454, c.num());