diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index d7463130ae..083ed7fad9 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -467,23 +467,27 @@ Number::Guard::doRoundUp( // be impossible to recurse more than once, because once the mantissa is divided by // 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 will have no // chance of bringing it back over. - if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && - mantissa > kMaxRep && mantissa < kMaxRepUp) + if (mantissa > kMaxRep && mantissa < kMaxRepUp) + { mantissa = kMaxRepUp; - doDropDigit(mantissa, exponent); - XRPL_ASSERT_PARTS( - safeToIncrement(mantissa), - "xrpl::Number::Guard::doRoundUp", - "can't recurse more than once"); - doRoundUp( - negative, - mantissa, - exponent, - minMantissa, - maxMantissa, - cuspRoundingFix, - location); - return; + } + else + { + doDropDigit(mantissa, exponent); + XRPL_ASSERT_PARTS( + safeToIncrement(mantissa), + "xrpl::Number::Guard::doRoundUp", + "can't recurse more than once"); + doRoundUp( + negative, + mantissa, + exponent, + minMantissa, + maxMantissa, + cuspRoundingFix, + location); + return; + } } } else @@ -501,7 +505,9 @@ Number::Guard::doRoundUp( } } } - else if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && mantissa > kMaxRep) + else if ( + cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && mantissa > kMaxRep && + mantissa < kMaxRepUp) { mantissa = kMaxRep; } @@ -559,7 +565,9 @@ Number::Guard::doRound( } ++drops; } - else if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && drops > kMaxRep) + else if ( + cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && drops > kMaxRep && + drops < kMaxRepUp) { // This will probably be impossible because this function is not called by mutating // functions, so the Number will already be normalized. @@ -614,7 +622,7 @@ doNormalize( { static constexpr auto kMinExponent = Number::kMinExponent; static constexpr auto kMaxExponent = Number::kMaxExponent; - auto const kRepLimit = cuspRoundingFix == MantissaRange::CuspRoundingFix::Disabled + auto const repLimit = cuspRoundingFix == MantissaRange::CuspRoundingFix::Disabled ? Number::kMaxRep : Number::kMaxRepUp; @@ -666,7 +674,7 @@ doNormalize( // 9,900,000,000,000,123,450 or 9,900,000,000,000,123,460. // mantissa() will return mantissa / 10, and exponent() will return // exponent + 1. - if (m > kRepLimit) + if (m > repLimit) { if (exponent >= kMaxExponent) throw std::overflow_error("Number::normalize 1.5"); @@ -676,7 +684,7 @@ doNormalize( // modification, it must be less than kMaxRep. In other words, the original // value should have been no more than kMaxRep * 10. // (kMaxRep * 10 > maxMantissa) - XRPL_ASSERT_PARTS(m <= kRepLimit, "xrpl::doNormalize", "intermediate mantissa fits in limit"); + XRPL_ASSERT_PARTS(m <= repLimit, "xrpl::doNormalize", "intermediate mantissa fits in limit"); mantissa = m; g.doRoundUp( @@ -804,7 +812,7 @@ Number::operator+=(Number const& y) auto const& minMantissa = range.min; auto const& maxMantissa = range.max; auto const cuspRoundingFix = range.cuspRoundingFix; - auto const kRepLimit = + auto const repLimit = cuspRoundingFix == MantissaRange::CuspRoundingFix::Disabled ? kMaxRep : kMaxRepUp; // Bring the exponents of both values into agreement, so the mantissas are on the same scale @@ -857,14 +865,14 @@ Number::operator+=(Number const& y) xm += ym; if (range.cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) { - while (xm > maxMantissa || xm > kRepLimit) + while (xm > maxMantissa || xm > repLimit) { g.doDropDigit(xm, xe); } } else { - if (xm > maxMantissa || xm > kRepLimit) + if (xm > maxMantissa || xm > repLimit) { g.doDropDigit(xm, xe); } @@ -884,7 +892,7 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - while (xm < minMantissa && xm * 10 <= kRepLimit) + while (xm < minMantissa && xm * 10 <= repLimit) { xm *= 10; xm -= g.pop(); @@ -897,7 +905,7 @@ Number::operator+=(Number const& y) Guard g; if (xn) g.setNegative(); - while (xm > maxMantissa || xm > kRepLimit) + while (xm > maxMantissa || xm > repLimit) { g.doDropDigit(xm, xe); } @@ -951,10 +959,10 @@ Number::operator*=(Number const& y) auto const& minMantissa = range.min; auto const& maxMantissa = range.max; auto const cuspRoundingFix = range.cuspRoundingFix; - auto const kRepLimit = + auto const repLimit = cuspRoundingFix == MantissaRange::CuspRoundingFix::Disabled ? kMaxRep : kMaxRepUp; - while (zm > maxMantissa || zm > kRepLimit) + while (zm > maxMantissa || zm > repLimit) { g.doDropDigit(zm, ze); } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 1fe88f2f85..6249362c8e 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -365,7 +365,7 @@ public: Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'000, -30}, __LINE__}}); - auto const cLarge = std::to_array( + auto const cLargeAll = std::to_array( // Note that items with extremely large mantissas need to be // calculated, because otherwise they overflow uint64. Items from C // with larger mantissa @@ -412,16 +412,55 @@ public: Number{1'000'000'000'000'000'000, -36}, __LINE__}, {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep - 1}, __LINE__}, - {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, - Number{1, 0}, - Number{(Number::kMaxRep / 10) + 1, 1}, - __LINE__}, - {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, - Number{3, 0}, - Number{Number::kMaxRep}, - __LINE__}, - {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}, __LINE__}, }); + // Note that items with extremely large mantissas need to be + // calculated, because otherwise they overflow uint64. Items from C + // with larger mantissa + auto const cLargeLegacy = std::to_array({ + // Anything larger than kMaxRep rounds up + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + {Number{false, Number::kMaxRep + 2, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 2, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}, __LINE__}, + }); + auto const cLarge = std::to_array({ + // kMaxRep + 1 is below the half-way point, so it rounds down to kMaxRep when the Number + // is created. + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{Number::kMaxRep - 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep - 3}, + __LINE__}, + // kMaxRepUp -1 is above the half-way point, so it rounds up to kMaxRepUp when the + // Number is created. Subtracting 1 from that rounds up again. A little non-intuitive. + {Number{false, Number::kMaxRepUp - 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + // Subtracting 3 gets back down to kMaxRep + {Number{false, Number::kMaxRepUp - 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + // 2^63 is the same as kMaxRep+1 + {power(2, 63), Number{3, 0}, Number{Number::kMaxRep - 3}, __LINE__}, + }); auto test = [this](auto const& c) { for (auto const& [x, y, z, line] : c) { @@ -431,13 +470,22 @@ public: expect(result == z, ss.str(), __FILE__, line); } }; - if (scale == MantissaRange::MantissaScale::Small) + switch (scale) { - test(cSmall); - } - else - { - test(cLarge); + case MantissaRange::MantissaScale::Small: + test(cSmall); + break; + case MantissaRange::MantissaScale::LargeLegacy: + test(cLargeAll); + test(cLargeLegacy); + break; + case MantissaRange::MantissaScale::Large: + test(cLargeAll); + test(cLarge); + break; + default: + BEAST_EXPECT(false); + break; } } @@ -1432,25 +1480,78 @@ public: "-9223372036854775807", __LINE__); - // Because the absolute value of min is larger than max, it - // will be scaled down to fit under max. Since we're - // rounding towards zero, the 8 at the end is dropped. - test( - Number{std::numeric_limits::min(), 0}, - "-9223372036854775800", - __LINE__); - test( - -(Number{std::numeric_limits::min(), 0}), - "9223372036854775800", - __LINE__); + switch (scale) + { + case MantissaRange::MantissaScale::LargeLegacy: + // Because the absolute value of min() is larger than max(), it + // will be scaled down to fit under max(). Since we're + // rounding towards zero, the 8 at the end is dropped. + test( + Number{std::numeric_limits::min(), 0}, + "-9223372036854775800", + __LINE__); + test( + -(Number{std::numeric_limits::min(), 0}), + "9223372036854775800", + __LINE__); + break; + case MantissaRange::MantissaScale::Large: + // Because the absolute value of min() is larger than max(), it + // will be rounded down toward max() + test( + Number{std::numeric_limits::min(), 0}, + "-9223372036854775807", + __LINE__); + test( + -(Number{std::numeric_limits::min(), 0}), + "9223372036854775807", + __LINE__); + break; + default: + BEAST_EXPECT(false); + break; + } } + switch (scale) + { + case MantissaRange::MantissaScale::LargeLegacy: + // Rounding to nearest, since the mantissa is bigger than kMaxRep, the 8 + // will be dropped, and since that is bigger than 5, the result will be + // rounded up from 0 to 1. + test( + Number{std::numeric_limits::max(), 0} + 1, + "9223372036854775810", + __LINE__); + test( + -(Number{std::numeric_limits::max(), 0} + 1), + "-9223372036854775810", + __LINE__); + break; + case MantissaRange::MantissaScale::Large: + // Rounding to nearest, since the mantissa is below the halfway point from + // kMaxRep to kMaxRep up, it will be rounded down to kMaxRep + test( + Number{std::numeric_limits::max(), 0} + 1, + "9223372036854775807", + __LINE__); + test( + -(Number{std::numeric_limits::max(), 0} + 1), + "-9223372036854775807", + __LINE__); + break; + default: + BEAST_EXPECT(false); + break; + } + // Rounding to nearest, since the mantissa is above the halfway point from kMaxRep + // to kMaxRep up, it will be rounded up to kMaxRepUp. test( - Number{std::numeric_limits::max(), 0} + 1, + Number{std::numeric_limits::max(), 0} + 2, "9223372036854775810", __LINE__); test( - -(Number{std::numeric_limits::max(), 0} + 1), + -(Number{std::numeric_limits::max(), 0} + 2), "-9223372036854775810", __LINE__); break; @@ -1701,7 +1802,7 @@ public: } void - testUpwardRoundsDown() + testEdgeCases() { auto const scale = Number::getMantissaScale(); { @@ -1725,15 +1826,14 @@ public: BigInt const signedDifference = storedValue - exactProduct; - log << "\n" - << " a = " << fmt(BigInt(kAValue)) << "\n" + log << " a = " << fmt(BigInt(kAValue)) << "\n" << " b = " << fmt(BigInt(kBValue)) << "\n" << " exact a*b = " << fmt(exactProduct) << "\n" << " stored = " << fmt(storedValue) << "\n" << " stored - exact = " << fmt(signedDifference) << "\n" << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n" << " stored.mantissa = " << product.mantissa() << "\n" - << " stored.exponent = " << product.exponent() << "\n"; + << " stored.exponent = " << product.exponent() << "\n\n"; log.flush(); switch (scale) @@ -1806,15 +1906,14 @@ public: dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); dec const diff = stored - exact; - log << "\n" - << " a = " << aValue << "\n" + log << " a = " << aValue << "\n" << " b = " << bValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (negative => Upward gave value BELOW truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; log.flush(); // Upward invariant: stored >= exact. Bug: stored < exact. @@ -1856,15 +1955,14 @@ public: dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); dec const diff = stored - exact; - log << "\n" - << " a = " << aValue << "\n" + log << " a = " << aValue << "\n" << " b = " << bValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (positive => Downward gave value ABOVE truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; log.flush(); // invariant: stored <= exact. Bug: stored > exact. @@ -1913,15 +2011,14 @@ public: dec const stored = dec(quotient.mantissa()) * pow10(quotient.exponent()); dec const diff = stored - exact; - log << "\n" - << " a = " << aValue << "\n" + log << " a = " << aValue << "\n" << " b = " << bValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (negative => ToNearest gave value BELOW truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; log.flush(); // invariant: stored >= exact. Bug: stored < exact. @@ -1966,8 +2063,7 @@ public: Number const downward = construct(Number::RoundingMode::Downward); - log << "\n" - << " actual = " << actual << " (kMaxRep + 1)\n" + log << " actual = " << actual << " (kMaxRep + 1)\n" << " below = " << below << " (kMaxRep, distance 1)\n" << " above = " << above << " (kMaxRep + 3, distance 2)\n" << " Upward = " << upward << "\n" @@ -2011,6 +2107,8 @@ public: break; default: + // Covers "Large" and any newly added scales + // Upward round UP BEAST_EXPECT(upward == above); @@ -2023,6 +2121,7 @@ public: // Both should have given the same answer, but they differ BEAST_EXPECT(toNearest == downward); + break; } } { @@ -2047,9 +2146,9 @@ public: BigInt const stored = toBigInt(sum); BigInt const diff = stored - exact; - log << "\n a = " << a << "\n b = " << b + log << " a = " << a << "\n b = " << b << "\n exact a + b = " << exact.str() << "\n TowardsZero = " << stored.str() - << "\n difference = " << diff.str() << "\n"; + << "\n difference = " << diff.str() << "\n\n"; log.flush(); if (scale == MantissaRange::MantissaScale::Small) @@ -2097,7 +2196,7 @@ public: testRounding(); testInt64(); - testUpwardRoundsDown(); + testEdgeCases(); } } };