From cc2406bf3ffc8d2d2fd063a17c454ce74e77ffc0 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 5 Feb 2026 19:13:14 -0500 Subject: [PATCH] fixup! fixup! Address review feedback from @copilot --- include/xrpl/basics/Number.h | 33 ++++++--------------------------- src/libxrpl/basics/Number.cpp | 14 ++++++++------ 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index d9ea81a7c1..c9b0cb2b3d 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -307,12 +307,6 @@ public: constexpr static int minExponent = -32768; constexpr static int maxExponent = 32768; -#if MAXREP - constexpr static internalrep maxRep = std::numeric_limits::max(); - static_assert(maxRep == 9'223'372'036'854'775'807); - static_assert(-maxRep == std::numeric_limits::min() + 1); -#endif - // May need to make unchecked private struct unchecked { @@ -537,10 +531,6 @@ private: static_assert(smallRange.max == 9'999'999'999'999'999LL); static_assert(smallRange.referenceMin == smallRange.min); static_assert(smallRange.log == 15); -#if MAXREP - static_assert(smallRange.min < maxRep); - static_assert(smallRange.max < maxRep); -#endif constexpr static MantissaRange largeRange{MantissaRange::large}; static_assert(!isPowerOfTen(largeRange.min)); static_assert(largeRange.min == 922'337'203'685'477'581ULL); @@ -556,11 +546,6 @@ private: // will be fine. If they don't, well need to bring them up into range. // Guard::bringIntoRange handles this situation. -#if MAXREP - static_assert(largeRange.min < maxRep); - static_assert(largeRange.max > maxRep); -#endif - // The range for the mantissa when normalized. // Use reference_wrapper to avoid making copies, and prevent accidentally // changing the values inside the range. @@ -616,10 +601,7 @@ private: Number shiftExponent(int exponentDelta) const; - // Safely convert rep (int64) mantissa to internalrep (uint64). If the rep - // is negative, returns the positive value. This takes a little extra work - // because converting std::numeric_limits::min() flirts with - // UB, and can vary across compilers. + // Safely return the absolute value of a rep (int64) mantissa as an internalrep (uint64). static internalrep externalToInternal(rep mantissa); @@ -676,7 +658,7 @@ public: }; inline constexpr Number::Number(bool negative, internalrep mantissa, int exponent, unchecked) noexcept - : mantissa_{(negative ? -1 : 1) * static_cast(mantissa)}, exponent_{exponent} + : mantissa_{negative ? -static_cast(mantissa) : static_cast(mantissa)}, exponent_{exponent} { } @@ -734,7 +716,7 @@ Number::operator-() const noexcept if (mantissa_ == 0) return Number{}; auto x = *this; - x.mantissa_ = -1 * x.mantissa_; + x.mantissa_ = -x.mantissa_; return x; } @@ -827,7 +809,7 @@ Number::lowest() noexcept inline bool Number::isnormal(MantissaRange const& range) const noexcept { - auto const abs_m = mantissa_ < 0 ? -mantissa_ : mantissa_; + auto const abs_m = externalToInternal(mantissa_); return *this == Number{} || (range.min <= abs_m && abs_m <= range.max && // @@ -846,7 +828,7 @@ Number::normalizeToRange(T minMantissa, T maxMantissa) const { bool negative = mantissa_ < 0; auto const sign = negative ? -1 : 1; - internalrep mantissa = sign * mantissa_; + internalrep mantissa = externalToInternal(mantissa_); int exponent = exponent_; if constexpr (std::is_unsigned_v) @@ -863,11 +845,8 @@ Number::normalizeToRange(T minMantissa, T maxMantissa) const // Cast mantissa to signed type first (if T is a signed type) to avoid // unsigned integer overflow when multiplying by negative sign - T signedMantissa = static_cast(mantissa); - if (negative) - signedMantissa = -signedMantissa; + T signedMantissa = negative ? static_cast(-mantissa) : static_cast(mantissa); return std::make_pair(signedMantissa, exponent); - return std::make_pair(sign * static_cast(mantissa), exponent); } inline constexpr Number diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index e279b7aa4d..a4600d52e2 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -281,9 +281,9 @@ Number::Guard::doRound(rep& drops, std::string_view location) { static_assert(sizeof(internalrep) == sizeof(rep)); // This should be impossible, because it's impossible to represent - // "maxRep + 0.6" in Number, regardless of the scale. There aren't - // enough digits available. You'd either get a mantissa of "maxRep" - // or "(maxRep + 1) / 10", neither of which will round up when + // "largestMantissa + 0.6" in Number, regardless of the scale. There aren't + // enough digits available. You'd either get a mantissa of "largestMantissa " + // or "largestMantissa / 10 + 1", neither of which will round up when // converting to rep, though the latter might overflow _before_ // rounding. throw std::overflow_error(std::string{location}); // LCOV_EXCL_LINE @@ -314,7 +314,7 @@ Number::externalToInternal(rep mantissa) /** Breaks down the number into components, potentially de-normalizing it. * - * Ensures that the mantissa always has range_.log digits. + * Ensures that the mantissa always has range_.log + 1 digits. * */ template @@ -393,7 +393,9 @@ Number::fromInternal(bool negative, Rep mantissa, int exponent, MantissaRange co auto const sign = negative ? -1 : 1; // mantissa is unsigned, but it might not be uint64 - mantissa_ = static_cast(sign * static_cast(mantissa)); + mantissa_ = static_cast(static_cast(mantissa)); + if (negative) + mantissa_ = -mantissa_; exponent_ = exponent; XRPL_ASSERT_PARTS( @@ -871,7 +873,7 @@ Number::operator rep() const if (drops < 0) { g.set_negative(); - drops = -drops; + drops = externalToInternal(drops); } for (; offset < 0; ++offset) {