Review feedback from @copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

include/xrpl/basics/Number.h

@@ -2,6 +2,7 @@
 
 #include <xrpl/beast/utility/instrumentation.h>
 
+#include <concepts>
 #include <cstdint>
 #include <functional>
 #include <limits>
@@ -25,13 +26,13 @@ to_string(Number const& amount);
  *
  * The return value is a pair (log, rem), where log is the estimated
  * base-10 logarithm (roughly floor(log10(value))), and rem is value with
- * all factors of 10 removed (i.e., divided by the largest power of 10 that
- * divides value). If rem is 1, then value is an exact power of ten, and
+ * all trailing 0s removed (i.e., divided by the largest power of 10 that
+ * evenly divides value). If rem is 1, then value is an exact power of ten, and
  * log is the exact log10(value).
  *
  * This function only works for positive values.
  */
-template <typename T>
+template <std::unsigned_integral T>
 constexpr std::pair<int, T>
 logTenEstimate(T value)
 {
@@ -99,20 +100,23 @@ struct MantissaRange
     explicit constexpr MantissaRange(mantissa_scale scale_)
         : max(getMax(scale_))
         , min(computeMin(max))
-        , referenceMin(getReferenceMin(scale_, min))
+        , internalMin(getInternalMin(scale_, min))
         , log(computeLog(min))
         , scale(scale_)
     {
-        // Since this is constexpr, if any of these throw, it won't compile
+        // Keep the error messages terse. Since this is constexpr, if any of these throw, it won't
+        // compile, so there's no real need to worry about runtime exceptions here.
         if (min * 10 <= max)
-            throw std::out_of_range("min * 10 <= max");
+            throw std::out_of_range("Invalid mantissa range: min * 10 <= max");
         if (max / 10 >= min)
-            throw std::out_of_range("max / 10 >= min");
+            throw std::out_of_range("Invalid mantissa range: max / 10 >= min");
         if ((min - 1) * 10 > max)
-            throw std::out_of_range("(min - 1) * 10 > max");
+            throw std::out_of_range("Invalid mantissa range: (min - 1) * 10 > max");
         // This is a little hacky
         if ((max + 10) / 10 < min)
-            throw std::out_of_range("(max + 10) / 10 < min");
+            throw std::out_of_range("Invalid mantissa range: (max + 10) / 10 < min");
+        if (computeLog(internalMin) != log)
+            throw std::out_of_range("Invalid mantissa range: computeLog(internalMin) != log");
     }
 
     // Explicitly delete copy and move operations
@@ -125,9 +129,12 @@ struct MantissaRange
 
     rep max;
     rep min;
-    // This is not a great name. Used to determine if mantissas are in range,
-    // but have fewer digits than max
-    rep referenceMin;
+    /* Used to determine if mantissas are in range, but have fewer digits than max.
+     *
+     * Unlike min, internalMin is always an exact power of 10, so a mantissa in the internal
+     * representation will always have a consistent number of digits.
+     */
+    rep internalMin;
     int log;
     mantissa_scale scale;
 
@@ -156,7 +163,7 @@ private:
     }
 
     static constexpr rep
-    getReferenceMin(mantissa_scale scale, rep min)
+    getInternalMin(mantissa_scale scale, rep min)
     {
         switch (scale)
         {
@@ -215,7 +222,7 @@ concept UnsignedMantissa = std::is_unsigned_v<T> || std::is_same_v<T, uint128_t>
  *
  * A non-zero mantissa is (almost) always normalized, meaning it and the
  * exponent are grown or shrunk until the mantissa is in the range
- * [MantissaRange.referenceMin, MantissaRange.referenceMin * 10 - 1].
+ * [MantissaRange.internalMin, MantissaRange.internalMin * 10 - 1].
  *
  * This internal representation is only used during some operations to ensure
  * that the mantissa is a known, predictable size. The class itself stores the
@@ -225,7 +232,7 @@ concept UnsignedMantissa = std::is_unsigned_v<T> || std::is_same_v<T, uint128_t>
  *   1. Normalization can be disabled by using the "unchecked" ctor tag. This
  *      should only be used at specific conversion points, some constexpr
  *      values, and in unit tests.
- *   2. Unlike MantissaRange.min, referenceMin is always an exact power of 10,
+ *   2. Unlike MantissaRange.min, internalMin is always an exact power of 10,
  *      so a mantissa in the internal representation will always have a
  *      consistent number of digits.
  *   3. The functions toInternal() and fromInternal() are used to convert
@@ -536,14 +543,14 @@ private:
     static_assert(isPowerOfTen(smallRange.min));
     static_assert(smallRange.min == 1'000'000'000'000'000LL);
     static_assert(smallRange.max == 9'999'999'999'999'999LL);
-    static_assert(smallRange.referenceMin == smallRange.min);
+    static_assert(smallRange.internalMin == smallRange.min);
     static_assert(smallRange.log == 15);
     constexpr static MantissaRange largeRange{MantissaRange::large};
     static_assert(!isPowerOfTen(largeRange.min));
     static_assert(largeRange.min == 922'337'203'685'477'581ULL);
     static_assert(largeRange.max == internalrep(9'223'372'036'854'775'807ULL));
     static_assert(largeRange.max == std::numeric_limits<rep>::max());
-    static_assert(largeRange.referenceMin == 1'000'000'000'000'000'000ULL);
+    static_assert(largeRange.internalMin == 1'000'000'000'000'000'000ULL);
     static_assert(largeRange.log == 18);
     // There are 2 values that will not fit in largeRange without some extra
     // work

src/libxrpl/basics/Number.cpp

@@ -110,7 +110,7 @@ public:
 
     // Modify the result to the correctly rounded value
     void
-    doRound(rep& drops, std::string_view location);
+    doRound(internalrep& drops, std::string_view location);
 
 private:
     void
@@ -222,9 +222,7 @@ Number::Guard::bringIntoRange(
     {
         constexpr Number zero = Number{};
 
-        negative = false;
-        mantissa = zero.mantissa_;
-        exponent = zero.exponent_;
+        std::tie(negative, mantissa, exponent) = zero.toInternal();
     }
 }
 
@@ -278,7 +276,7 @@ Number::Guard::doRoundDown(
 
 // Modify the result to the correctly rounded value
 void
-Number::Guard::doRound(rep& drops, std::string_view location)
+Number::Guard::doRound(internalrep& drops, std::string_view location)
 {
     auto r = round();
     if (r == 1 || (r == 0 && (drops & 1) == 1))
@@ -297,8 +295,6 @@ Number::Guard::doRound(rep& drops, std::string_view location)
         }
         ++drops;
     }
-    if (is_negative())
-        drops = -drops;
 }
 
 // Number
@@ -314,8 +310,10 @@ Number::externalToInternal(rep mantissa)
     if (mantissa >= 0)
         return mantissa;
 
-    // Cast to unsigned before negating to avoid undefined behavior
-    // when v == INT64_MIN (negating INT64_MIN in signed is UB)
+    // Cast to unsigned before negating to avoid undefined behavior when
+    // mantissa == std::numeric_limits<rep>::min() (INT64_MIN). Negating
+    // INT64_MIN in signed arithmetic is UB, but casting to the unsigned
+    // internalrep first makes the operation well-defined.
     return -static_cast<internalrep>(mantissa);
 }
 
@@ -334,16 +332,16 @@ Number::toInternal(MantissaRange const& range) const
     // case.
     Rep mantissa = static_cast<Rep>(externalToInternal(mantissa_));
 
-    auto const referenceMin = range.referenceMin;
+    auto const internalMin = range.internalMin;
     auto const minMantissa = range.min;
 
-    if (mantissa != 0 && mantissa >= minMantissa && mantissa < referenceMin)
+    if (mantissa != 0 && mantissa >= minMantissa && mantissa < internalMin)
     {
         // Ensure the mantissa has the correct number of digits
         mantissa *= 10;
         --exponent;
         XRPL_ASSERT_PARTS(
-            mantissa >= referenceMin && mantissa < referenceMin * 10,
+            mantissa >= internalMin && mantissa < internalMin * 10,
             "xrpl::Number::toInternal()",
             "Number is within reference range and has 'log' digits");
     }
@@ -439,7 +437,7 @@ constexpr Number
 Number::oneSmall()
 {
     return Number{
-        false, Number::smallRange.referenceMin, -Number::smallRange.log, Number::unchecked{}};
+        false, Number::smallRange.internalMin, -Number::smallRange.log, Number::unchecked{}};
 };
 
 constexpr Number oneSml = Number::oneSmall();
@@ -448,7 +446,7 @@ constexpr Number
 Number::oneLarge()
 {
     return Number{
-        false, Number::largeRange.referenceMin, -Number::largeRange.log, Number::unchecked{}};
+        false, Number::largeRange.internalMin, -Number::largeRange.log, Number::unchecked{}};
 };
 
 constexpr Number oneLrg = Number::oneLarge();
@@ -484,11 +482,10 @@ doNormalize(
     using Guard = Number::Guard;
 
     constexpr Number zero = Number{};
+    auto const& range = Number::range_.get();
     if (mantissa == 0 || (mantissa < minMantissa && exponent <= minExponent))
     {
-        mantissa = zero.mantissa_;
-        exponent = zero.exponent_;
-        negative = false;
+        std::tie(negative, mantissa, exponent) = zero.toInternal(range);
         return;
     }
 
@@ -511,9 +508,7 @@ doNormalize(
     }
     if ((exponent < minExponent) || (m == 0))
     {
-        mantissa = zero.mantissa_;
-        exponent = zero.exponent_;
-        negative = false;
+        std::tie(negative, mantissa, exponent) = zero.toInternal(range);
         return;
     }
 
@@ -888,30 +883,36 @@ Number::operator/=(Number const& y)
 Number::
 operator rep() const
 {
-    rep drops = mantissa();
+    auto const m = mantissa();
+    internalrep drops = externalToInternal(m);
+
+    if (drops == 0)
+        return drops;
+
     int offset = exponent();
     Guard g;
-    if (drops != 0)
+
+    if (m < 0)
     {
-        if (drops < 0)
-        {
-            g.set_negative();
-            drops = externalToInternal(drops);
-        }
-        for (; offset < 0; ++offset)
-        {
-            g.push(drops % 10);
-            drops /= 10;
-        }
-        for (; offset > 0; --offset)
-        {
-            if (drops >= largeRange.min)
-                throw std::overflow_error("Number::operator rep() overflow");
-            drops *= 10;
-        }
-        g.doRound(drops, "Number::operator rep() rounding overflow");
+        g.set_negative();
     }
-    return drops;
+    for (; offset < 0; ++offset)
+    {
+        g.push(drops % 10);
+        drops /= 10;
+    }
+    for (; offset > 0; --offset)
+    {
+        if (drops >= largeRange.min)
+            throw std::overflow_error("Number::operator rep() overflow");
+        drops *= 10;
+    }
+    g.doRound(drops, "Number::operator rep() rounding overflow");
+
+    if (g.is_negative())
+        return -drops;
+    else
+        return drops;
 }
 
 Number
@@ -1068,7 +1069,7 @@ Number::root(MantissaRange const& range, Number f, unsigned d)
         return f;
 
     auto const [e, di] = [&]() {
-        auto const [negative, mantissa, exponent] = f.toInternal(range);
+        auto const exponent = std::get<2>(f.toInternal(range));
 
         // Scale f into the range (0, 1) such that the scale change (e) is a
         // multiple of the root (d)
@@ -1157,7 +1158,7 @@ root2(Number f)
         return f;
 
     auto const e = [&]() {
-        auto const [negative, mantissa, exponent] = f.toInternal(range);
+        auto const exponent = std::get<2>(f.toInternal(range));
 
         // Scale f into the range (0, 1) such that f's exponent is a
         // multiple of d

src/test/basics/Number_test.cpp

@@ -311,6 +311,15 @@ public:
             test(cLarge);
     }
 
+    static std::uint64_t
+    getMaxInternalMantissa()
+    {
+        return static_cast<std::uint64_t>(
+                   static_cast<std::int64_t>(power(10, Number::mantissaLog()))) *
+            10 -
+            1;
+    }
+
     void
     test_mul()
     {
@@ -335,10 +344,7 @@ public:
                 test(cLarge);
         };
         auto const maxMantissa = Number::maxMantissa();
-        auto const maxInternalMantissa = static_cast<std::uint64_t>(static_cast<std::int64_t>(
-                                             power(10, Number::mantissaLog()))) *
-                10 -
-            1;
+        auto const maxInternalMantissa = getMaxInternalMantissa();
 
         saveNumberRoundMode save{Number::setround(Number::to_nearest)};
         {
@@ -594,13 +600,13 @@ public:
                      Number{10, 0},
                      __LINE__},
                     // Maximum internal mantissa range - rounds down to
-                    // maxMantissa/10-1
+                    // maxInternalMantissa/10-1
                     // 99'999'999'999'999'999'800'000'000'000'000'000'100
                     {Number{false, maxInternalMantissa, 0, Number::normalized{}},
                      Number{false, maxInternalMantissa, 0, Number::normalized{}},
                      Number{false, maxInternalMantissa / 10 - 1, 20, Number::normalized{}},
                      __LINE__},
-                    // Maximum mantissa range - same as int64
+                    // Maximum external mantissa range - same as INT64_MAX (2^63-1)
                     {Number{false, maxMantissa, 0, Number::normalized{}},
                      Number{false, maxMantissa, 0, Number::normalized{}},
                      Number{85'070'591'730'234'615'84, 19},
@@ -931,10 +937,7 @@ public:
         };
         */
 
-        auto const maxInternalMantissa = static_cast<std::uint64_t>(static_cast<std::int64_t>(
-                                             power(10, Number::mantissaLog()))) *
-                10 -
-            1;
+        auto const maxInternalMantissa = getMaxInternalMantissa();
 
         auto const cSmall = std::to_array<Case>(
             {{Number{2}, 2, Number{1414213562373095049, -18}},
@@ -1005,7 +1008,7 @@ public:
             }
         };
 
-        auto const maxInternalMantissa = power(10, Number::mantissaLog()) * 10 - 1;
+        Number const maxInternalMantissa = power(10, Number::mantissaLog()) * 10 - 1;
 
         auto const cSmall = std::to_array<Number>({
             Number{2},
@@ -1642,11 +1645,7 @@ public:
             NumberRoundModeGuard mg(Number::towards_zero);
 
             {
-                auto const maxInternalMantissa =
-                    static_cast<std::uint64_t>(
-                        static_cast<std::int64_t>(power(10, Number::mantissaLog()))) *
-                        10 -
-                    1;
+                auto const maxInternalMantissa = getMaxInternalMantissa();
 
                 // Rounds down to fit under 2^63
                 Number const max = Number{false, maxInternalMantissa, 0, Number::normalized{}};