ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-03 00:36:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Review feedback from Tapanito and AI

Browse Source 
- Add missing headers.
- Improve code coverage exclusions.
- Clean up several variable names.
- Improve explanatory comments.
- Remove the switch statement from MantissaRange::getMin. Change it to
  a straight power of ten lookup.
This commit is contained in:
Ed Hennis
2026-05-28 18:37:15 -04:00
parent 06a3f76ccd
commit 2fdfd2b420
2 changed files with 43 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
include/xrpl/basics/Number.h
View File

@@ -9,6 +9,7 @@
#include <optional>
#include <ostream>
#include <set>
#include <stdexcept>
#include <string>
#include <unordered_map>
@@ -50,11 +51,11 @@ namespace detail {
* compile time. Doing it at runtime would be pretty wasteful and
* inefficient.
*/
constexpr std::size_t int64digits = 20;
consteval std::array<std::uint64_t, int64digits>
constexpr std::size_t kInt64Digits = 20;
consteval std::array<std::uint64_t, kInt64Digits>
buildPowersOfTen()
{
std::array<std::uint64_t, int64digits> result{};
std::array<std::uint64_t, kInt64Digits> result{};
std::uint64_t power = 1;
std::size_t exponent = 0;
@@ -74,13 +75,13 @@ buildPowersOfTen()
} // namespace detail
constexpr std::array<std::uint64_t, detail::int64digits> kPowerOfTen = detail::buildPowersOfTen();
constexpr std::array<std::uint64_t, detail::kInt64Digits> kPowerOfTen = detail::buildPowersOfTen();
static_assert(kPowerOfTen[0] == 1);
static_assert(kPowerOfTen[1] == 10);
static_assert(kPowerOfTen[10] == 10'000'000'000);
static_assert(
isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::int64digits - 1);
isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kInt64Digits - 1);
/** MantissaRange defines a range for the mantissa of a normalized Number.
*
@@ -159,10 +160,12 @@ private:
case MantissaScale::LargeLegacy:
case MantissaScale::Large:
return 18;
// LCOV_EXCL_START
default:
// If called in a constexpr context, this throw assures that the build fails if an
// invalid scale is used.
throw std::runtime_error("Unknown mantissa scale"); // LCOV_EXCL_LINE
throw std::runtime_error("Unknown mantissa scale");
// LCOV_EXCL_STOP
}
}
@@ -171,17 +174,9 @@ private:
static constexpr rep
getMin(MantissaScale scale, int exponent)
{
switch (scale)
{
case MantissaScale::Small:
case MantissaScale::LargeLegacy:
case MantissaScale::Large:
return kPowerOfTen[exponent];
default:
// If called in a constexpr context, this throw assures that the build fails if an
// invalid scale is used.
throw std::runtime_error("Unknown mantissa scale"); // LCOV_EXCL_LINE
}
if (exponent < 0 || exponent >= kPowerOfTen.size())
throw std::runtime_error("Invalid exponent"); // LCOV_EXCL_LINE
return kPowerOfTen[exponent];
}
static constexpr CuspRoundingFix

42
src/libxrpl/basics/Number.cpp
View File

@@ -622,8 +622,12 @@ Number::normalize<uint128_t>(
internalrep const& maxMantissa,
MantissaRange::CuspRoundingFix cuspRoundingFixEnabled)
{
// Not used by every compiler version, and thus not necessarily
// counted by coverage build
// LCOV_EXCL_START
doNormalize(
negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false);
// LCOV_EXCL_STOP
}
template <>
@@ -636,8 +640,12 @@ Number::normalize<unsigned long long>(
internalrep const& maxMantissa,
MantissaRange::CuspRoundingFix cuspRoundingFixEnabled)
{
// Not used by every compiler version, and thus not necessarily
// counted by coverage build
// LCOV_EXCL_START
doNormalize(
negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false);
// LCOV_EXCL_STOP
}
template <>
@@ -852,7 +860,9 @@ Number::operator/=(Number const& y)
return *this;
// n* = numerator
// d* = denominator
// *p = negative (positive?)
// z* = result (quotient)
// *p = negative (p for positive, even though the value means not
// positive?)
// *s = sign
// *m = mantissa
// *e = exponent
@@ -876,7 +886,10 @@ Number::operator/=(Number const& y)
// Division operates on two large integers (16-digit for small
// mantissas, 19-digit for large) using integer math. If the values
// were just divided directly, the result would be at most 9.
// were just divided directly, the result would be only ever be one
// digit or zero - not very useful.
// e.g. 9'876'543'210'987'654 / 1'234'567'890'123'456 = 8
// 1'234'567'890'123'456 / 9'876'543'210'987'654 = 0
// Introduce a power-of-ten multiplication factor for the numerator
// which will ensure the result has a meaningful number of digits.
//
@@ -885,11 +898,11 @@ Number::operator/=(Number const& y)
// * 23 / 67 = 0
// * Use a factor of 10^4
// * 230'000 / 67 = 3432 with an exponent of -4
// * The normalized result will be 34, exponent -2
// * The normalized result will be 34, exponent -2, or 0.34
//
// The most extreme results are 10/99 and 99/10
// * 100'000 / 99 = 1'010e-4 = 10e-2
// * 990'000 / 10 = 99'000e-4 = 99e-1
// * 100'000 / 99 = 1'010e-4 = 10e-2 or 0.10
// * 990'000 / 10 = 99'000e-4 = 99e-1 or 9.9
//
// Note that the computations give 2 or 3 digits after the
// decimal point to determine which way to round for most scenarios.
@@ -899,8 +912,7 @@ Number::operator/=(Number const& y)
// behavior, which must not be changed.)
//
// For large mantissas (where the MantissaRange.log == 18), a shift by 10^20 would be optimal
// for most scenarios. However, larger mantissa values would overflow 2^128 (log(2^128,10)
// ~ 38.5).
// for most scenarios. However, larger mantissa values would overflow 2^128.
//
// * log(2^128,10) ~ 38.5
// * largeRange.log = 18, fits in 10^19
@@ -927,6 +939,8 @@ Number::operator/=(Number const& y)
// remainder is exactly 0.5 for "ToNearest". This will give the
// rounding the most accurate result possible, as if infinite
// precision was used in the initial calculation.
// Stage 1: Do the initial division with a factor of 10^17.
auto constexpr factorExponent = 17;
uint128_t constexpr f = kPowerOfTen[factorExponent];
@@ -935,7 +949,7 @@ Number::operator/=(Number const& y)
auto zm = numerator / dm;
auto ze = ne - de - factorExponent;
bool zn = (ns * ds) < 0;
bool zp = (ns * ds) < 0;
// dropped is used in the same way as Guard::xbit_. In the case of
// division, it indicates if there's any remainder left over after
// we have been as precise as reasonable. If there is, it would be as
@@ -947,7 +961,7 @@ Number::operator/=(Number const& y)
{
// Stage 2
//
// If there is a remainder, treat is as a secondary numerator.
// If there is a remainder, treat it as a secondary numerator.
// Multiply by correctionFactor separately from stage 1.
// The math for this would work for small mantissas, but we need to
// preserve legacy behavior.
@@ -980,6 +994,12 @@ Number::operator/=(Number const& y)
auto const partialNumerator = remainder * correctionFactor;
auto const correction = partialNumerator / dm;
// If the correction is zero, we do not have to make any
// modifications to z*, because it will not have any
// effect on the final result. (We'd be adding a bunch of
// zeros to the end of zm that would just be removed in
// normalize.) However, if that is the case, then Stage 3 is
// even more important for accuracy.
if (correction != 0)
{
zm *= correctionFactor;
@@ -1001,8 +1021,8 @@ Number::operator/=(Number const& y)
}
}
}
doNormalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped);
negative_ = zn;
doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped);
negative_ = zp;
mantissa_ = static_cast<internalrep>(zm);
exponent_ = ze;
XRPL_ASSERT_PARTS(isnormal(), "xrpl::Number::operator/=", "result is normalized");