Fix clang-tidy issues, and more AI complaints

This commit is contained in:
Ed Hennis
2026-05-06 23:26:11 -04:00
parent a2b21d75ce
commit b050c151f8
5 changed files with 79 additions and 61 deletions

View File

@@ -50,7 +50,7 @@ isPowerOfTen(T value)
* options. This intentionally restricts the number of unique MantissaRanges that can
* be instantiated: one for each scale.
*
* The "small" scale is based on the behavior of STAmount for IOUs. It has a min
* The "Small" scale is based on the behavior of STAmount for IOUs. It has a min
* value of 10^15, and a max value of 10^16-1. This was sufficient for
* uses before Lending Protocol was implemented, mostly related to AMM.
*
@@ -61,12 +61,16 @@ isPowerOfTen(T value)
* STNumber field type, and for internal calculations. That necessitated the
* "large" scale.
*
* The "large" scale is intended to represent all values that can be represented
* The "Large" scales are intended to represent all values that can be represented
* by an STAmount - IOUs, XRP, and MPTs. It has a min value of 10^18, and a max
* value of 10^19-1.
* value of 10^19-1. "LargeLegacy" is like "Large", but preserves
* a rounding error when a computation results in a mantissa of
* Number::kMAX_REP that needs to be rounded up, but rounds down
* instead. It will maintain consistent behavior until the fixCleanup3_2_0
* amendment is enabled.
*
* Note that if the mentioned amendments are eventually retired, this class
* should be left in place, but the "small" scale option should be removed. This
* should be left in place, but the "Small" scale option should be removed. This
* will allow for future expansion beyond 64-bits if it is ever needed.
*/
struct MantissaRange
@@ -125,9 +129,9 @@ private:
}
static constexpr CuspRoundingFix
isCuspFixEnabled(MantissaScale scale_)
isCuspFixEnabled(MantissaScale scale)
{
switch (scale_)
switch (scale)
{
case MantissaScale::Small:
case MantissaScale::LargeLegacy:
@@ -473,10 +477,10 @@ public:
one();
template <
auto minMantissa,
auto maxMantissa,
Integral64 T = std::decay_t<decltype(minMantissa)>,
Integral64 TMax = std::decay_t<decltype(maxMantissa)>>
auto MinMantissa,
auto MaxMantissa,
Integral64 T = std::decay_t<decltype(MinMantissa)>,
Integral64 TMax = std::decay_t<decltype(MaxMantissa)>>
[[nodiscard]]
std::pair<T, int>
normalizeToRange() const;
@@ -723,18 +727,18 @@ Number::isnormal() const noexcept
kMIN_EXPONENT <= exponent_ && exponent_ <= kMAX_EXPONENT);
}
template <auto minMantissa, auto maxMantissa, Integral64 T, Integral64 TMax>
template <auto MinMantissa, auto MaxMantissa, Integral64 T, Integral64 TMax>
std::pair<T, int>
Number::normalizeToRange() const
{
static_assert(std::is_same_v<T, std::uint64_t> || std::is_same_v<T, std::int64_t>);
static_assert(std::is_same_v<T, TMax>);
auto constexpr min = static_cast<T>(minMantissa);
auto constexpr max = static_cast<T>(maxMantissa);
static_assert(min > 0);
static_assert(min % 10 == 0);
static_assert(max % 10 == 9);
static_assert((max + 1) / 10 == min);
auto constexpr kMIN = static_cast<T>(MinMantissa);
auto constexpr kMAX = static_cast<T>(MaxMantissa);
static_assert(kMIN > 0);
static_assert(kMIN % 10 == 0);
static_assert(kMAX % 10 == 9);
static_assert((kMAX + 1) / 10 == kMIN);
bool negative = negative_;
internalrep mantissa = mantissa_;
@@ -750,7 +754,7 @@ Number::normalizeToRange() const
// Don't need to worry about the cuspRounding fix because rounding up will never take the
// mantissa over maxMantissa with a ones digit value other than 0. 0 can safely be truncated.
Number::normalize(
negative, mantissa, exponent, min, max, MantissaRange::CuspRoundingFix::Disabled);
negative, mantissa, exponent, kMIN, kMAX, MantissaRange::CuspRoundingFix::Disabled);
auto const sign = negative ? -1 : 1;
return std::make_pair(static_cast<T>(sign * mantissa), exponent);
@@ -801,11 +805,11 @@ to_string(MantissaRange::MantissaScale const& scale)
switch (scale)
{
case MantissaRange::MantissaScale::Small:
return "Small";
return "small";
case MantissaRange::MantissaScale::LargeLegacy:
return "LargeLegacy";
return "largeLegacy";
case MantissaRange::MantissaScale::Large:
return "Large";
return "large";
default:
throw std::runtime_error("Bad scale");
}

View File

@@ -10,9 +10,11 @@
#include <iterator>
#include <limits>
#include <numeric>
#include <set>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <unordered_map>
#include <utility>
#ifdef _MSC_VER
@@ -34,18 +36,18 @@ thread_local std::reference_wrapper<MantissaRange const> Number::kRANGE =
std::set<MantissaRange::MantissaScale> const&
MantissaRange::getAllScales()
{
static std::set<MantissaRange::MantissaScale> const scales = {
static std::set<MantissaRange::MantissaScale> const kSCALES = {
MantissaRange::MantissaScale::Small,
MantissaRange::MantissaScale::LargeLegacy,
MantissaRange::MantissaScale::Large,
};
return scales;
return kSCALES;
}
std::unordered_map<MantissaRange::MantissaScale, MantissaRange> const&
MantissaRange::getRanges()
{
static auto const map = []() {
static auto const kMAP = []() {
std::unordered_map<MantissaScale, MantissaRange> map;
for (auto const scale : getAllScales())
{
@@ -56,41 +58,41 @@ MantissaRange::getRanges()
// created correctly, but nothing else.
{
[[maybe_unused]]
constexpr static MantissaRange range{MantissaRange::MantissaScale::Small};
static_assert(isPowerOfTen(range.min));
static_assert(range.min == 1'000'000'000'000'000LL);
static_assert(range.max == 9'999'999'999'999'999LL);
static_assert(range.log == 15);
static_assert(range.min < Number::kMAX_REP);
static_assert(range.max < Number::kMAX_REP);
static_assert(range.cuspRoundingFixEnabled == CuspRoundingFix::Disabled);
constexpr static MantissaRange kRANGE{MantissaRange::MantissaScale::Small};
static_assert(isPowerOfTen(kRANGE.min));
static_assert(kRANGE.min == 1'000'000'000'000'000LL);
static_assert(kRANGE.max == 9'999'999'999'999'999LL);
static_assert(kRANGE.log == 15);
static_assert(kRANGE.min < Number::kMAX_REP);
static_assert(kRANGE.max < Number::kMAX_REP);
static_assert(kRANGE.cuspRoundingFixEnabled == CuspRoundingFix::Disabled);
}
{
[[maybe_unused]]
constexpr static MantissaRange range{MantissaRange::MantissaScale::LargeLegacy};
static_assert(isPowerOfTen(range.min));
static_assert(range.min == 1'000'000'000'000'000'000ULL);
static_assert(range.max == rep(9'999'999'999'999'999'999ULL));
static_assert(range.log == 18);
static_assert(range.min < Number::kMAX_REP);
static_assert(range.max > Number::kMAX_REP);
static_assert(range.cuspRoundingFixEnabled == CuspRoundingFix::Disabled);
constexpr static MantissaRange kRANGE{MantissaRange::MantissaScale::LargeLegacy};
static_assert(isPowerOfTen(kRANGE.min));
static_assert(kRANGE.min == 1'000'000'000'000'000'000ULL);
static_assert(kRANGE.max == rep(9'999'999'999'999'999'999ULL));
static_assert(kRANGE.log == 18);
static_assert(kRANGE.min < Number::kMAX_REP);
static_assert(kRANGE.max > Number::kMAX_REP);
static_assert(kRANGE.cuspRoundingFixEnabled == CuspRoundingFix::Disabled);
}
{
[[maybe_unused]]
constexpr static MantissaRange range{MantissaRange::MantissaScale::Large};
static_assert(isPowerOfTen(range.min));
static_assert(range.min == 1'000'000'000'000'000'000ULL);
static_assert(range.max == rep(9'999'999'999'999'999'999ULL));
static_assert(range.log == 18);
static_assert(range.min < Number::kMAX_REP);
static_assert(range.max > Number::kMAX_REP);
static_assert(range.cuspRoundingFixEnabled == CuspRoundingFix::Enabled);
constexpr static MantissaRange kRANGE{MantissaRange::MantissaScale::Large};
static_assert(isPowerOfTen(kRANGE.min));
static_assert(kRANGE.min == 1'000'000'000'000'000'000ULL);
static_assert(kRANGE.max == rep(9'999'999'999'999'999'999ULL));
static_assert(kRANGE.log == 18);
static_assert(kRANGE.min < Number::kMAX_REP);
static_assert(kRANGE.max > Number::kMAX_REP);
static_assert(kRANGE.cuspRoundingFixEnabled == CuspRoundingFix::Enabled);
}
return map;
}();
return map;
return kMAP;
}
MantissaRange const&

View File

@@ -38,12 +38,20 @@ setCurrentTransactionRules(std::optional<Rules> r)
// Make global changes associated with the rules before the value is moved.
// Push the appropriate setting, instead of having the class pull every time
// the value is needed. That could get expensive fast.
// The improved accuracy that will come from enabling large
// mantissas is a goal separate from SAV and LP. It was originally
// only tied to those two amendments to avoid needing a new
// amendment of it's own, and because they require that behavior.
// Because fixCleanup3_2_0 fixes a separate bug related to the large
// mantissas, that can take precedence and activate the large
// mantissas even in the absence of the other two amendments.
bool const enableCuspRoundingFix = !r || r->enabled(fixCleanup3_2_0);
bool const enableLargeNumbers = enableCuspRoundingFix ||
bool const enableVaultNumbers = enableCuspRoundingFix ||
(r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol));
Number::setMantissaScale(
enableCuspRoundingFix ? MantissaRange::MantissaScale::Large
: enableLargeNumbers ? MantissaRange::MantissaScale::LargeLegacy
: enableVaultNumbers ? MantissaRange::MantissaScale::LargeLegacy
: MantissaRange::MantissaScale::Small);
*getCurrentTransactionRulesRef() = std::move(r);

View File

@@ -65,7 +65,7 @@ namespace xrpl::test {
/**
* Tests of AMM that use offers too.
*/
struct AMMExtended_test : public jtx::AMMTest
class AMMExtended_test : public jtx::AMMTest
{
// Use small Number mantissas for the life of this test.
NumberMantissaScaleGuard const sg{xrpl::MantissaRange::MantissaScale::Small};

View File

@@ -31,7 +31,7 @@ class Number_test : public beast::unit_test::Suite
int count = 0;
for (auto it = s.rbegin(); it != s.rend(); ++it)
{
if (count && count % 3 == 0)
if (count != 0 && count % 3 == 0)
out.insert(out.begin(), '_');
out.insert(out.begin(), *it);
++count;
@@ -222,7 +222,7 @@ public:
{Number{Number::kMAX_REP}, Number{6, -1}, Number{Number::kMAX_REP / 10, 1}},
});
auto const cLargeCorrected = std::to_array<Case>({
{Number{Number::kMAX_REP}, Number{6, -1}, Number{Number::kMAX_REP / 10 + 1, 1}},
{Number{Number::kMAX_REP}, Number{6, -1}, Number{(Number::kMAX_REP / 10) + 1, 1}},
});
auto test = [this](auto const& c) {
for (auto const& [x, y, z] : c)
@@ -241,9 +241,13 @@ public:
{
test(cLarge);
if (scale == MantissaRange::MantissaScale::LargeLegacy)
{
test(cLargeLegacy);
}
else
{
test(cLargeCorrected);
}
}
{
bool caught = false;
@@ -1589,16 +1593,16 @@ public:
NumberMantissaScaleGuard const mg{MantissaRange::MantissaScale::Large};
NumberRoundModeGuard const rg{Number::RoundingMode::Upward};
constexpr std::int64_t aValue = 1'000'000'000'000'049'863LL;
constexpr std::int64_t bValue = 9'223'372'036'854'315'903LL;
constexpr std::int64_t kA_VALUE = 1'000'000'000'000'049'863LL;
constexpr std::int64_t kB_VALUE = 9'223'372'036'854'315'903LL;
// Public conversion operator: STAmount::operator Number() const.
Number const a = aValue;
Number const b = bValue;
Number const a = kA_VALUE;
Number const b = kB_VALUE;
Number const product = a * b;
// Exact reference in BigInt.
BigInt const exactProduct = BigInt(aValue) * BigInt(bValue);
BigInt const exactProduct = BigInt(kA_VALUE) * BigInt(kB_VALUE);
// What Number actually stored.
BigInt storedValue = BigInt(product.mantissa());
@@ -1608,8 +1612,8 @@ public:
BigInt const signedDifference = storedValue - exactProduct;
log << "\n"
<< " a = " << fmt(BigInt(aValue)) << "\n"
<< " b = " << fmt(BigInt(bValue)) << "\n"
<< " a = " << fmt(BigInt(kA_VALUE)) << "\n"
<< " b = " << fmt(BigInt(kB_VALUE)) << "\n"
<< " exact a*b = " << fmt(exactProduct) << "\n"
<< " stored = " << fmt(storedValue) << "\n"
<< " stored - exact = " << fmt(signedDifference) << "\n"