From 9dd8f3e3502d6f4c7435cf16a692b717c9dad311 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 28 Jul 2026 13:58:26 +0100 Subject: [PATCH 1/5] Simplest fixes --- src/benchmarks/libxrpl/nodestore/Backend.cpp | 15 +++-- .../libxrpl/nodestore/NodeStoreBench.h | 9 +-- src/tests/libxrpl/basics/IntrusiveShared.cpp | 66 +++++++++---------- src/tests/libxrpl/basics/MallocTrim.cpp | 2 +- src/tests/libxrpl/basics/base58.cpp | 22 +++---- src/tests/libxrpl/basics/base_uint.cpp | 38 ++++++----- .../libxrpl/consensus/CensorshipDetector.cpp | 2 +- src/tests/libxrpl/csf/TrustGraph.h | 4 +- src/tests/libxrpl/csf/random.h | 9 +-- src/tests/libxrpl/resource/Logic.cpp | 19 ++++-- src/tests/libxrpl/shamap/SHAMap.cpp | 17 ++--- src/tests/libxrpl/shamap/SHAMapSync.cpp | 29 ++++---- 12 files changed, 125 insertions(+), 107 deletions(-) diff --git a/src/benchmarks/libxrpl/nodestore/Backend.cpp b/src/benchmarks/libxrpl/nodestore/Backend.cpp index f854dbd3a77..2dab1155d06 100644 --- a/src/benchmarks/libxrpl/nodestore/Backend.cpp +++ b/src/benchmarks/libxrpl/nodestore/Backend.cpp @@ -20,9 +20,10 @@ namespace xrpl::node_store { namespace { -constexpr std::size_t kPoolSizes[] = {1000, 10000, 100000}; -constexpr int kThreadCounts[] = {1, 4, 8}; +constexpr auto kPoolSizes = std::to_array({1000, 10000, 100000}); +constexpr auto kThreadCounts = std::to_array({1, 4, 8}); constexpr std::size_t kBatchSize = 256; +constexpr std::size_t kMissRatio = 5; constexpr std::string_view kNamePrefix = "BM_Backend_"; constexpr std::string_view kNameSeparator = "/"; @@ -142,7 +143,7 @@ Workload const kMixed{ auto& [rs, backend, index, poolSize] = ctx; std::shared_ptr result; auto const pick = rs.shuffle[index % poolSize]; - if (index % 5 == 0) + if (index % kMissRatio == 0) { backend.fetch(rs.missing[pick], &result); } @@ -239,7 +240,7 @@ registerWorkload(BackendConfig const& bc, Workload const& w) { auto rs = std::make_shared(); auto* b = benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs)); - b->RangeMultiplier(10)->Range(kPoolSizes[0], kPoolSizes[std::size(kPoolSizes) - 1]); + b->RangeMultiplier(10)->Range(kPoolSizes.front(), kPoolSizes.back()); b->Threads(1)->Threads(4)->Threads(8)->UseRealTime(); return; @@ -249,14 +250,14 @@ registerWorkload(BackendConfig const& bc, Workload const& w) { for (auto const threads : kThreadCounts) { - if (poolSize % static_cast(threads) != 0) + if (poolSize % threads != 0) continue; auto rs = std::make_shared(); benchmark::RegisterBenchmark(name, makeRunner(w, cfg, rs)) ->Arg(poolSize) - ->Iterations(poolSize / static_cast(threads)) - ->Threads(threads) + ->Iterations(poolSize / threads) + ->Threads(static_cast(threads)) ->UseRealTime(); } } diff --git a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h index 57abf42e89b..4d971b36093 100644 --- a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h +++ b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -145,7 +146,7 @@ makePool(std::uint8_t prefix, std::size_t count, std::size_t start = 0) Sequence seq(prefix); Batch pool; pool.reserve(count); - for (std::size_t i = 0; i < count; ++i) + for (auto const i : std::views::iota(0uz, count)) pool.push_back(seq.obj(start + i)); return pool; } @@ -158,7 +159,7 @@ makeMissingKeys(std::size_t count) Sequence seq(2); std::vector keys; keys.reserve(count); - for (std::size_t i = 0; i < count; ++i) + for (auto const i : std::views::iota(0uz, count)) keys.push_back(seq.key(i)); return keys; } @@ -206,9 +207,9 @@ inline std::vector makeShuffle(std::size_t size, std::uint64_t seed) { std::vector v(size); - std::iota(v.begin(), v.end(), std::size_t{0}); + std::ranges::iota(v, 0uz); beast::xor_shift_engine gen(seed); - std::shuffle(v.begin(), v.end(), gen); + std::ranges::shuffle(v, gen); return v; } diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index e798cd1cccc..eea25eb0cea 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -50,11 +50,11 @@ struct Barrier { std::mutex mtx; std::condition_variable cv; - int count; - int const initial; + std::size_t count; + std::size_t const initial; std::size_t generation{0}; - explicit Barrier(int n) : count(n), initial(n) + explicit Barrier(std::size_t n) : count(n), initial(n) { } @@ -217,7 +217,7 @@ TEST(IntrusiveSharedTest, basics) auto id = b->id; EXPECT_EQ(TIBase::getState(id), Alive); EXPECT_EQ(b->useCount(), 1); - for (int i = 0; i < 10; ++i) + for (auto i = 0uz; i < 10; ++i) { strong.push_back(b); } @@ -232,7 +232,7 @@ TEST(IntrusiveSharedTest, basics) id = b->id; EXPECT_EQ(TIBase::getState(id), Alive); EXPECT_EQ(b->useCount(), 1); - for (int i = 0; i < 10; ++i) + for (auto i = 0uz; i < 10; ++i) { weak.emplace_back(b); EXPECT_EQ(b->useCount(), 1); @@ -506,11 +506,11 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) auto createVecOfPointers = [&](auto const& toClone, std::default_random_engine& eng) -> std::vector, WeakIntrusive>> { std::vector, WeakIntrusive>> result; - std::uniform_int_distribution<> toCreateDist(4, 64); + std::uniform_int_distribution toCreateDist(4, 64); std::uniform_int_distribution<> isStrongDist(0, 1); auto numToCreate = toCreateDist(eng); result.reserve(numToCreate); - for (int i = 0; i < numToCreate; ++i) + for (auto i = 0uz; i < numToCreate; ++i) { if (isStrongDist(eng)) { @@ -523,8 +523,8 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) } return result; }; - constexpr int kLoopIters = 2 * 1024; - constexpr int kNumThreads = 16; + constexpr auto kLoopIters = 2uz * 1024; + constexpr auto kNumThreads = 16uz; std::vector> toClone; Barrier loopStartSyncPoint{kNumThreads}; Barrier postCreateToCloneSyncPoint{kNumThreads}; @@ -533,7 +533,7 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) std::random_device rd; std::vector result; result.reserve(kNumThreads); - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) result.emplace_back(rd()); return result; }(); @@ -541,8 +541,8 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) // cloneAndDestroy clones the strong pointer into a vector of mixed // strong and weak pointers and destroys them all at once. // threadId==0 is special. - auto cloneAndDestroy = [&](int threadId) { - for (int i = 0; i < kLoopIters; ++i) + auto cloneAndDestroy = [&](std::size_t threadId) { + for (auto i = 0uz; i < kLoopIters; ++i) { // ------ Sync Point ------ loopStartSyncPoint.arriveAndWait(); @@ -582,11 +582,11 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) }; std::vector threads; threads.reserve(kNumThreads); - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads.emplace_back(cloneAndDestroy, i); } - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads[i].join(); } @@ -638,16 +638,16 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) [&](auto const& toClone, std::default_random_engine& eng) -> std::vector> { std::vector> result; - std::uniform_int_distribution<> toCreateDist(4, 64); + std::uniform_int_distribution toCreateDist(4, 64); auto numToCreate = toCreateDist(eng); result.reserve(numToCreate); - for (int i = 0; i < numToCreate; ++i) + for (auto i = 0uz; i < numToCreate; ++i) result.emplace_back(SharedIntrusive(toClone)); return result; }; - constexpr int kLoopIters = 2 * 1024; - constexpr int kFlipPointersLoopIters = 256; - constexpr int kNumThreads = 16; + constexpr auto kLoopIters = 2uz * 1024; + constexpr auto kFlipPointersLoopIters = 256uz; + constexpr auto kNumThreads = 16uz; std::vector> toClone; Barrier loopStartSyncPoint{kNumThreads}; Barrier postCreateToCloneSyncPoint{kNumThreads}; @@ -657,7 +657,7 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) std::random_device rd; std::vector result; result.reserve(kNumThreads); - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) result.emplace_back(rd()); return result; }(); @@ -666,8 +666,8 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) // mixed strong and weak pointers, runs a loop that randomly // changes strong pointers to weak pointers, and destroys them // all at once. - auto cloneAndDestroy = [&](int threadId) { - for (int i = 0; i < kLoopIters; ++i) + auto cloneAndDestroy = [&](std::size_t threadId) { + for (auto i = 0uz; i < kLoopIters; ++i) { // ------ Sync Point ------ loopStartSyncPoint.arriveAndWait(); @@ -702,7 +702,7 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) postCreateVecOfPointersSyncPoint.arriveAndWait(); std::uniform_int_distribution<> isStrongDist(0, 1); - for (int f = 0; f < kFlipPointersLoopIters; ++f) + for (auto f = 0uz; f < kFlipPointersLoopIters; ++f) { for (auto& p : v) { @@ -725,11 +725,11 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) }; std::vector threads; threads.reserve(kNumThreads); - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads.emplace_back(cloneAndDestroy, i); } - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads[i].join(); } @@ -773,9 +773,9 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) } }; - constexpr int kLoopIters = 2 * 1024; - constexpr int kLockWeakLoopIters = 256; - constexpr int kNumThreads = 16; + constexpr auto kLoopIters = 2uz * 1024; + constexpr auto kLockWeakLoopIters = 256uz; + constexpr auto kNumThreads = 16uz; std::vector> toLock; Barrier loopStartSyncPoint{kNumThreads}; Barrier postCreateToLockSyncPoint{kNumThreads}; @@ -784,8 +784,8 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) // lockAndDestroy creates weak pointers from the strong pointer // and runs a loop that locks the weak pointer. At the end of the loop // all the pointers are destroyed all at once. - auto lockAndDestroy = [&](int threadId) { - for (int i = 0; i < kLoopIters; ++i) + auto lockAndDestroy = [&](std::size_t threadId) { + for (auto i = 0uz; i < kLoopIters; ++i) { // ------ Sync Point ------ loopStartSyncPoint.arriveAndWait(); @@ -816,7 +816,7 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) // Multiple threads all create a weak pointer from the same // strong pointer WeakIntrusive const weak{toLock[threadId]}; - for (int wi = 0; wi < kLockWeakLoopIters; ++wi) + for (auto wi = 0uz; wi < kLockWeakLoopIters; ++wi) { EXPECT_FALSE(weak.expired()); auto strong = weak.lock(); @@ -831,11 +831,11 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) }; std::vector threads; threads.reserve(kNumThreads); - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads.emplace_back(lockAndDestroy, i); } - for (int i = 0; i < kNumThreads; ++i) + for (auto i = 0uz; i < kNumThreads; ++i) { threads[i].join(); } diff --git a/src/tests/libxrpl/basics/MallocTrim.cpp b/src/tests/libxrpl/basics/MallocTrim.cpp index 6ac8957f0eb..52151262b01 100644 --- a/src/tests/libxrpl/basics/MallocTrim.cpp +++ b/src/tests/libxrpl/basics/MallocTrim.cpp @@ -199,7 +199,7 @@ TEST(mallocTrim, repeated_calls) beast::Journal const journal{beast::Journal::getNullSink()}; // Call malloc_trim multiple times to ensure it's safe - for (int i = 0; i < 5; ++i) + for (auto i = 0uz; i < 5; ++i) { MallocTrimReport const report = mallocTrim("iteration_" + std::to_string(i), journal); diff --git a/src/tests/libxrpl/basics/base58.cpp b/src/tests/libxrpl/basics/base58.cpp index d452453f767..d6b1d2c3f9c 100644 --- a/src/tests/libxrpl/basics/base58.cpp +++ b/src/tests/libxrpl/basics/base58.cpp @@ -151,7 +151,7 @@ randomBigInt(std::uint8_t minSize = 1, std::uint8_t maxSize = 5) auto const numCoeff = numCoeffDist(eng); std::vector coeffs; coeffs.reserve(numCoeff); - for (int i = 0; i < numCoeff; ++i) + for (auto i = 0uz; i < numCoeff; ++i) { coeffs.push_back(dist(eng)); } @@ -167,7 +167,7 @@ TEST(Base58Test, multiprecision) auto eng = randEngine(); std::uniform_int_distribution dist; std::uniform_int_distribution dist1(1); - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::uint64_t const d = dist(eng); if (d == 0u) @@ -185,7 +185,7 @@ TEST(Base58Test, multiprecision) EXPECT_EQ(refMod.convert_to(), mod); EXPECT_EQ(foundDiv, refDiv); } - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::uint64_t const d = dist(eng); auto bigInt = multiprecision_utils::randomBigInt(/*minSize*/ 2); @@ -204,7 +204,7 @@ TEST(Base58Test, multiprecision) auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); EXPECT_EQ(refAdd, foundAdd); } - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::uint64_t const d = dist1(eng); // Force overflow @@ -221,7 +221,7 @@ TEST(Base58Test, multiprecision) auto const foundAdd = multiprecision_utils::toBoostMP(bigInt); EXPECT_NE(refAdd, foundAdd); } - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::uint64_t const d = dist(eng); auto bigInt = multiprecision_utils::randomBigInt(/* minSize */ 2); @@ -239,7 +239,7 @@ TEST(Base58Test, multiprecision) auto const foundMul = multiprecision_utils::toBoostMP(bigInt); EXPECT_EQ(refMul, foundMul); } - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::uint64_t const d = dist1(eng); // Force overflow @@ -265,7 +265,7 @@ TEST(Base58Test, fast_matches_ref) std::array b256ResultBuf[2]; std::array, 2> b256Result; - for (int i = 0; i < 2; ++i) + for (auto i = 0uz; i < 2; ++i) { std::span const outBuf{b58ResultBuf[i]}; if (i == 0) @@ -297,7 +297,7 @@ TEST(Base58Test, fast_matches_ref) } } - for (int i = 0; i < 2; ++i) + for (auto i = 0uz; i < 2; ++i) { std::span const outBuf{b256ResultBuf[i].data(), b256ResultBuf[i].size()}; if (i == 0) @@ -339,7 +339,7 @@ TEST(Base58Test, fast_matches_ref) std::array b256ResultBuf[2]; std::array, 2> b256Result; - for (int i = 0; i < 2; ++i) + for (auto i = 0uz; i < 2; ++i) { std::span const outBuf{b58ResultBuf[i].data(), b58ResultBuf[i].size()}; if (i == 0) @@ -370,7 +370,7 @@ TEST(Base58Test, fast_matches_ref) } } - for (int i = 0; i < 2; ++i) + for (auto i = 0uz; i < 2; ++i) { std::span const outBuf{b256ResultBuf[i].data(), b256ResultBuf[i].size()}; if (i == 0) @@ -425,7 +425,7 @@ TEST(Base58Test, fast_matches_ref) // test with random data constexpr std::size_t kIters = 100000; - for (int i = 0; i < kIters; ++i) + for (auto i = 0uz; i < kIters; ++i) { std::array b256DataBuf{}; auto const [tokType, b256Data] = randomB256TestData(b256DataBuf); diff --git a/src/tests/libxrpl/basics/base_uint.cpp b/src/tests/libxrpl/basics/base_uint.cpp index 174cc33aa04..525e7c0407b 100644 --- a/src/tests/libxrpl/basics/base_uint.cpp +++ b/src/tests/libxrpl/basics/base_uint.cpp @@ -59,14 +59,17 @@ struct BaseUintTest : public ::testing::Test static void testComparisons() { + using HexPair = std::pair; + { - static constexpr std::array, 6> kTestArgs{ - {{"0000000000000000", "0000000000000001"}, - {"0000000000000000", "ffffffffffffffff"}, - {"1234567812345678", "2345678923456789"}, - {"8000000000000000", "8000000000000001"}, - {"aaaaaaaaaaaaaaa9", "aaaaaaaaaaaaaaaa"}, - {"fffffffffffffffe", "ffffffffffffffff"}}}; + static constexpr auto kTestArgs = std::to_array({ + {"0000000000000000", "0000000000000001"}, + {"0000000000000000", "ffffffffffffffff"}, + {"1234567812345678", "2345678923456789"}, + {"8000000000000000", "8000000000000001"}, + {"aaaaaaaaaaaaaaa9", "aaaaaaaaaaaaaaaa"}, + {"fffffffffffffffe", "ffffffffffffffff"}, + }); for (auto const& arg : kTestArgs) { @@ -91,15 +94,14 @@ struct BaseUintTest : public ::testing::Test } { - static constexpr std::array, 6> kTestArgs{ - { - {"000000000000000000000000", "000000000000000000000001"}, - {"000000000000000000000000", "ffffffffffffffffffffffff"}, - {"0123456789ab0123456789ab", "123456789abc123456789abc"}, - {"555555555555555555555555", "55555555555a555555555555"}, - {"aaaaaaaaaaaaaaa9aaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaa"}, - {"fffffffffffffffffffffffe", "ffffffffffffffffffffffff"}, - }}; + static constexpr auto kTestArgs = std::to_array({ + {"000000000000000000000000", "000000000000000000000001"}, + {"000000000000000000000000", "ffffffffffffffffffffffff"}, + {"0123456789ab0123456789ab", "123456789abc123456789abc"}, + {"555555555555555555555555", "55555555555a555555555555"}, + {"aaaaaaaaaaaaaaa9aaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaa"}, + {"fffffffffffffffffffffffe", "ffffffffffffffffffffffff"}, + }); for (auto const& arg : kTestArgs) { @@ -401,14 +403,14 @@ TEST_F(BaseUintTest, base_uint) { } }; - constexpr StrBaseUInt kTestCases[] = { + constexpr auto kTestCases = std::to_array({ "000000000000000000000000", "000000000000000000000001", "fedcba9876543210ABCDEF91", "19FEDCBA0123456789abcdef", "800000000000000000000000", "fFfFfFfFfFfFfFfFfFfFfFfF", - }; + }); for (StrBaseUInt const& t : kTestCases) { diff --git a/src/tests/libxrpl/consensus/CensorshipDetector.cpp b/src/tests/libxrpl/consensus/CensorshipDetector.cpp index aa6b2d086be..2c6b6ec731b 100644 --- a/src/tests/libxrpl/consensus/CensorshipDetector.cpp +++ b/src/tests/libxrpl/consensus/CensorshipDetector.cpp @@ -69,7 +69,7 @@ TEST(CensorshipDetectorTest, censorship_detector) runRound(cdet, ++round, {23, 24, 25, 26}, {25, 27}, {23, 26}, {24}); runRound(cdet, ++round, {23, 26, 28}, {26, 28}, {23}, {}); - for (int i = 0; i != 10; ++i) + for (auto i = 0uz; i != 10; ++i) runRound(cdet, ++round, {23}, {}, {23}, {}); runRound(cdet, ++round, {23, 29}, {29}, {23}, {}); diff --git a/src/tests/libxrpl/csf/TrustGraph.h b/src/tests/libxrpl/csf/TrustGraph.h index d010b954e08..8a804fcd5bc 100644 --- a/src/tests/libxrpl/csf/TrustGraph.h +++ b/src/tests/libxrpl/csf/TrustGraph.h @@ -118,9 +118,9 @@ class TrustGraph std::vector res; // Loop over all pairs of uniqueUNLs - for (int i = 0; i < uniqueUNLs.size(); ++i) + for (auto i = 0uz; i < uniqueUNLs.size(); ++i) { - for (int j = (i + 1); j < uniqueUNLs.size(); ++j) + for (auto j = i + 1; j < uniqueUNLs.size(); ++j) { auto const& unlA = uniqueUNLs[i]; auto const& unlB = uniqueUNLs[j]; diff --git a/src/tests/libxrpl/csf/random.h b/src/tests/libxrpl/csf/random.h index 007bdecb1b6..56838bb2806 100644 --- a/src/tests/libxrpl/csf/random.h +++ b/src/tests/libxrpl/csf/random.h @@ -24,11 +24,12 @@ randomWeightedShuffle(std::vector v, std::vector w, G& g) { using std::swap; - for (int i = 0; i < v.size() - 1; ++i) + for (auto i = 0uz; i + 1 < v.size(); ++i) { - // pick a random item weighted by w - std::discrete_distribution<> dd(w.begin() + i, w.end()); // NOLINT(misc-const-correctness) - auto idx = dd(g); + // Pick a random item from the unplaced tail, weighted by w. + // NOLINTNEXTLINE(misc-const-correctness) + std::discrete_distribution dd(w.begin() + i, w.end()); + auto const idx = i + dd(g); std::swap(v[i], v[idx]); std::swap(w[i], w[idx]); } diff --git a/src/tests/libxrpl/resource/Logic.cpp b/src/tests/libxrpl/resource/Logic.cpp index 1f935ebf4b3..383977c896b 100644 --- a/src/tests/libxrpl/resource/Logic.cpp +++ b/src/tests/libxrpl/resource/Logic.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -175,6 +176,8 @@ TEST_F(ResourceManagerTest, unlimited_warn_drop) TEST_F(ResourceManagerTest, charges) { + static constexpr auto kDecayTicks = 128uz; + TestLogic logic{j_}; { @@ -183,7 +186,7 @@ TEST_F(ResourceManagerTest, charges) Charge const fee{1000}; JLOG(j_.info()) << "Charging " << c.toString() << " " << fee << " per second"; c.charge(fee); - for (int i = 0; i < 128; ++i) + for (auto tick = 0uz; tick < kDecayTicks; ++tick) { JLOG(j_.info()) << "Time= " << logic.clock().now().time_since_epoch().count() << ", Balance = " << c.balance(); @@ -196,7 +199,7 @@ TEST_F(ResourceManagerTest, charges) Consumer c{logic.newInboundEndpoint(address)}; Charge const fee{1000}; JLOG(j_.info()) << "Charging " << c.toString() << " " << fee << " per second"; - for (int i = 0; i < 128; ++i) + for (auto tick = 0uz; tick < kDecayTicks; ++tick) { c.charge(fee); JLOG(j_.info()) << "Time= " << logic.clock().now().time_since_epoch().count() @@ -210,13 +213,15 @@ TEST_F(ResourceManagerTest, imports) { TestLogic logic{j_}; - Gossip g[5]; + static constexpr auto kGossipSources = 5uz; + + std::array gossip; - for (auto& i : g) - populateGossip(i); + for (auto& g : gossip) + populateGossip(g); - for (int i = 0; i < 5; ++i) - logic.importConsumers(std::to_string(i), g[i]); + for (auto i = 0uz; i < gossip.size(); ++i) + logic.importConsumers(std::to_string(i), gossip[i]); } TEST_F(ResourceManagerTest, import) diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 238c34bf9c4..e662e16be4e 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -257,12 +257,9 @@ TEST_P(SHAMapTest, add_traverse_snapshot_build_tear_and_iterate) map.invariants(); } - int h = 7; + auto keyIndex = kKeys.size(); for (auto const& k : map) - { - EXPECT_EQ(k.key(), kKeys[h]); - --h; - } + EXPECT_EQ(k.key(), kKeys[--keyIndex]); } } @@ -288,7 +285,11 @@ TEST_F(SHAMapPathProof, verify_proof_path) uint256 rootHash; std::vector goodPath; - for (unsigned char c = 1; c < 100; ++c) + static constexpr unsigned char kFirstKey = 1; + static constexpr unsigned char kKeyCount = 100; + static constexpr unsigned char kLastKey = kKeyCount - 1; + + for (unsigned char c = kFirstKey; c < kKeyCount; ++c) { uint256 k(c); map.addItem(SHAMapNodeType::TnAccountState, makeShamapitem(k, Slice{k.data(), k.size()})); @@ -304,7 +305,7 @@ TEST_F(SHAMapPathProof, verify_proof_path) auto& proofPath = *path; EXPECT_TRUE(map.verifyProofPath(root, k, proofPath)); - if (c == 1) + if (c == kFirstKey) { // extra node proofPath.insert(proofPath.begin(), proofPath.front()); @@ -313,7 +314,7 @@ TEST_F(SHAMapPathProof, verify_proof_path) uint256 const wrongKey(c + 1); EXPECT_FALSE(map.getProofPath(wrongKey)); } - if (c == 99) + if (c == kLastKey) { key = k; rootHash = root; diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index 5cefbae8a16..be23895626b 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -34,15 +35,17 @@ class SHAMapSyncTest : public ::testing::Test boost::intrusive_ptr makeRandomAS() { + static constexpr auto kWordsPerState = 3uz; + Serializer s; - for (int d = 0; d < 3; ++d) + for (auto word = 0uz; word < kWordsPerState; ++word) s.add32(randInt(eng_)); return makeShamapitem(s.getSHA512Half(), s.slice()); } bool - confuseMap(SHAMap& map, int count) + confuseMap(SHAMap& map, std::size_t count) { // add a bunch of random states to a map, then remove them // map should be the same @@ -50,7 +53,7 @@ class SHAMapSyncTest : public ::testing::Test std::list items; - for (int i = 0; i < count; ++i) + for (auto i = 0uz; i < count; ++i) { auto item = makeRandomAS(); items.push_back(item->key()); @@ -87,26 +90,30 @@ TEST_F(SHAMapSyncTest, sync) SHAMap source{SHAMapType::FREE, f}; SHAMap destination{SHAMapType::FREE, f2}; - int const items = 10000; - for (int i = 0; i < items; ++i) + static constexpr auto kItemCount = 10000uz; + static constexpr auto kInvariantInterval = 100uz; + static constexpr auto kNodesToConfuse = 500uz; + static constexpr auto kMaxNodesPerRequest = 2048; + + for (auto i = 0uz; i < kItemCount; ++i) { source.addItem(SHAMapNodeType::TnAccountState, makeRandomAS()); - if (i % 100 == 0) + if (i % kInvariantInterval == 0) source.invariants(); } source.invariants(); - ASSERT_TRUE(confuseMap(source, 500)); + ASSERT_TRUE(confuseMap(source, kNodesToConfuse)); source.invariants(); source.setImmutable(); - int count = 0; + std::size_t count = 0; source.visitLeaves([&count]([[maybe_unused]] auto const& item) { ++count; }); - EXPECT_EQ(count, items); + EXPECT_EQ(count, kItemCount); std::vector missingNodes; - source.walkMap(missingNodes, 2048); + source.walkMap(missingNodes, kMaxNodesPerRequest); EXPECT_TRUE(missingNodes.empty()); destination.setSynching(); @@ -127,7 +134,7 @@ TEST_F(SHAMapSyncTest, sync) f.clock().advance(std::chrono::seconds(1)); // get the list of nodes we know we need - auto nodesMissing = destination.getMissingNodes(2048, nullptr); + auto nodesMissing = destination.getMissingNodes(kMaxNodesPerRequest, nullptr); if (nodesMissing.empty()) break; From 6e71c03273999227d84420b43c464017869d0f29 Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 28 Jul 2026 15:20:24 +0100 Subject: [PATCH 2/5] More small fixes --- cmake/XrplAddBenchmark.cmake | 2 ++ src/benchmarks/libxrpl/nodestore/Backend.cpp | 24 +++++++++---------- .../libxrpl/nodestore/NodeStoreBench.h | 22 ++++++----------- src/tests/libxrpl/basics/IntrusiveShared.cpp | 22 ++++++++--------- src/tests/libxrpl/basics/Number.cpp | 16 ++++--------- src/tests/libxrpl/basics/base_uint.cpp | 8 +++---- src/tests/libxrpl/basics/join.cpp | 4 ++-- src/tests/libxrpl/nodestore/Database.cpp | 6 ++--- src/tests/libxrpl/resource/Logic.cpp | 17 ++++++------- 9 files changed, 54 insertions(+), 67 deletions(-) diff --git a/cmake/XrplAddBenchmark.cmake b/cmake/XrplAddBenchmark.cmake index 1dd875dd61b..921deb06587 100644 --- a/cmake/XrplAddBenchmark.cmake +++ b/cmake/XrplAddBenchmark.cmake @@ -1,3 +1,5 @@ +include_guard() + include(isolate_headers) # Define a benchmark executable for the module `name`. diff --git a/src/benchmarks/libxrpl/nodestore/Backend.cpp b/src/benchmarks/libxrpl/nodestore/Backend.cpp index 2dab1155d06..c739013b88d 100644 --- a/src/benchmarks/libxrpl/nodestore/Backend.cpp +++ b/src/benchmarks/libxrpl/nodestore/Backend.cpp @@ -30,12 +30,12 @@ constexpr std::string_view kNameSeparator = "/"; struct RunState { - std::unique_ptr harness; - Batch present; // prefix-1 objects, eligible to be stored - Batch recent; // prefix-1 objects in the "future" key space - std::vector missing; // prefix-2 keys that are never stored - std::vector shuffle; // [0, poolSize) permutation for random-like access - std::size_t avgPayload = 0; // mean getData().size() over `present` + std::unique_ptr harness; ///< backend under test, rebuilt per run + Batch present; ///< prefix-1 objects, eligible to be stored + Batch recent; ///< prefix-1 objects in the "future" key space + std::vector missing; ///< prefix-2 keys that are never stored + std::vector shuffle; ///< [0, poolSize) permutation for random-like access + std::size_t avgPayload = 0; ///< mean getData().size() over `present` void release() @@ -86,7 +86,7 @@ Workload const kInsert{ }, .iterate = [](IterateContext const& ctx) { - auto& [rs, backend, index, poolSize] = ctx; + auto const& [rs, backend, index, poolSize] = ctx; backend.store(rs.present[index % poolSize]); }, .reportBytes = true, @@ -105,7 +105,7 @@ Workload const kFetch{ }, .iterate = [](IterateContext const& ctx) { - auto& [rs, backend, index, poolSize] = ctx; + auto const& [rs, backend, index, poolSize] = ctx; std::shared_ptr result; backend.fetch(rs.present[index % poolSize]->getHash(), &result); benchmark::DoNotOptimize(result); @@ -119,7 +119,7 @@ Workload const kMissing{ .setup = [](SetupContext const& ctx) { ctx.rs.missing = makeMissingKeys(ctx.poolSize); }, .iterate = [](IterateContext const& ctx) { - auto& [rs, backend, index, poolSize] = ctx; + auto const& [rs, backend, index, poolSize] = ctx; std::shared_ptr result; backend.fetch(rs.missing[index % poolSize], &result); benchmark::DoNotOptimize(result); @@ -140,7 +140,7 @@ Workload const kMixed{ }, .iterate = [](IterateContext const& ctx) { - auto& [rs, backend, index, poolSize] = ctx; + auto const& [rs, backend, index, poolSize] = ctx; std::shared_ptr result; auto const pick = rs.shuffle[index % poolSize]; if (index % kMissRatio == 0) @@ -171,7 +171,7 @@ Workload const kWork{ }, .iterate = [](IterateContext const& ctx) { - auto& [rs, backend, index, poolSize] = ctx; + auto const& [rs, backend, index, poolSize] = ctx; auto const slot = index % poolSize; auto const pick = rs.shuffle[slot]; @@ -290,7 +290,7 @@ registerStoreBatch(BackendConfig const& bc) rs->harness = std::make_unique(cfg); rs->present = makePool(1, poolSize); rs->avgPayload = averagePayload(rs->present); - std::vector const batches = sliceBatches(rs->present, kBatchSize); + std::vector const batches = slicePreciseBatches(rs->present, kBatchSize); if (batches.empty()) { state.SkipWithError("pool smaller than one batch"); diff --git a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h index 4d971b36093..2b00341fc47 100644 --- a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h +++ b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h @@ -41,18 +41,13 @@ inline void rngcpy(void* buffer, std::size_t bytes, Generator& g) { using result_type = typename Generator::result_type; - while (bytes >= sizeof(result_type)) + while (bytes > 0) { auto const v = g(); - std::memcpy(buffer, &v, sizeof(v)); - buffer = reinterpret_cast(buffer) + sizeof(v); - bytes -= sizeof(v); - } - - if (bytes > 0) - { - auto const v = g(); - std::memcpy(buffer, &v, bytes); + auto const chunk = std::min(bytes, sizeof(result_type)); + std::memcpy(buffer, &v, chunk); + buffer = reinterpret_cast(buffer) + chunk; + bytes -= chunk; } } @@ -216,7 +211,7 @@ makeShuffle(std::size_t size, std::uint64_t seed) // Partition a pool into fixed-size batches. Any trailing remainder shorter than // `batchSize` is dropped, so every returned batch has exactly `batchSize`. inline std::vector -sliceBatches(Batch const& pool, std::size_t batchSize) +slicePreciseBatches(Batch const& pool, std::size_t batchSize) { std::vector batches; if (batchSize == 0) @@ -229,13 +224,10 @@ sliceBatches(Batch const& pool, std::size_t batchSize) /** * @brief RAII owner of a NodeStore Backend opened on a private temporary directory. - * - * Member declaration order matters: `tempDir` is declared first so it is - * destroyed last, after the backend has closed and released its files. */ struct BackendHarness { - beast::TempDir tempDir; + beast::TempDir tempDir; ///< Declared first so it is destroyed last DummyScheduler scheduler; beast::Journal journal{beast::Journal::getNullSink()}; std::unique_ptr backend; diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index eea25eb0cea..a8dd3006d6a 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -280,17 +280,17 @@ TEST(IntrusiveSharedTest, basics) TIBase::ResetStatesGuard const rsg{true}; using enum TrackedState; - using swu = SharedWeakUnion; - swu b = makeSharedIntrusive(); + using SharedWeak = SharedWeakUnion; + SharedWeak b = makeSharedIntrusive(); EXPECT_TRUE(b.isStrong() && b.useCount() == 1); auto id = b.get()->id; EXPECT_EQ(TIBase::getState(id), Alive); - swu w = b; + SharedWeak w = b; EXPECT_TRUE(TIBase::getState(id) == Alive); EXPECT_TRUE(w.isStrong() && b.useCount() == 2); w.convertToWeak(); EXPECT_TRUE(w.isWeak() && b.useCount() == 1); - swu s = w; + SharedWeak s = w; EXPECT_TRUE(s.isWeak() && b.useCount() == 1); s.convertToStrong(); EXPECT_TRUE(s.isStrong() && b.useCount() == 2); @@ -391,7 +391,7 @@ TEST(IntrusiveSharedTest, partial_delete) // given an opportunity to run during partial delete. EXPECT_EQ(cur, PartiallyDeleted); } - if (next == PartiallyDeletedStarted) + else if (next == PartiallyDeletedStarted) { partialDeleteStartedSyncPoint.arrive_and_wait(); using namespace std::chrono_literals; @@ -400,11 +400,11 @@ TEST(IntrusiveSharedTest, partial_delete) // is running. The test is to make sure that doesn't happen. std::this_thread::sleep_for(800ms); } - if (next == PartiallyDeleted) + else if (next == PartiallyDeleted) { EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); } - if (next == Deleted) + else if (next == Deleted) { EXPECT_FALSE(destructorRan.exchange(true)); } @@ -448,7 +448,7 @@ TEST(IntrusiveSharedTest, destructor) { EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); } - if (next == Deleted) + else if (next == Deleted) { EXPECT_FALSE(destructorRan.exchange(true)); } @@ -497,7 +497,7 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) EXPECT_FALSE(partialDeleteRan || destructorRan); setPartialDeleteRan(); } - if (next == Deleted) + else if (next == Deleted) { EXPECT_FALSE(destructorRan); setDestructorRan(); @@ -628,7 +628,7 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) EXPECT_FALSE(partialDeleteRan || destructorRan); setPartialDeleteRan(); } - if (next == Deleted) + else if (next == Deleted) { EXPECT_FALSE(destructorRan); setDestructorRan(); @@ -766,7 +766,7 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) EXPECT_FALSE(partialDeleteRan || destructorRan); setPartialDeleteRan(); } - if (next == Deleted) + else if (next == Deleted) { EXPECT_FALSE(destructorRan); setDestructorRan(); diff --git a/src/tests/libxrpl/basics/Number.cpp b/src/tests/libxrpl/basics/Number.cpp index 36e1b4a700b..32f93eb1f7a 100644 --- a/src/tests/libxrpl/basics/Number.cpp +++ b/src/tests/libxrpl/basics/Number.cpp @@ -323,11 +323,11 @@ TEST(NumberTest, add) __LINE__, }, { - // Does not round. Mantissas are going to be > maxRep, so if + // Does not round. Mantissas are going to be > kMaxRep, so if // added together as uint64_t's, the result will overflow. // With addition using uint128_t, there's no problem. After // normalizing, the resulting mantissa ends up less than - // maxRep. + // kMaxRep. Number{false, 9'999'999'999'999'999'990ULL, 0, Number::Normalized{}}, Number{false, 9'999'999'999'999'999'990ULL, 0, Number::Normalized{}}, Number{false, 1'999'999'999'999'999'998ULL, 1, Number::Normalized{}}, @@ -1078,14 +1078,6 @@ TEST(NumberTest, root) EXPECT_EQ(result, z) << ss.str(); } }; - /* - auto tests = [&](auto const& cSmall, auto const& cLarge) { - test(cSmall); - if (scale != MantissaRange::mantissa_scale::small) - test(cLarge); - }; - */ - auto const cSmall = std::to_array( {{Number{2}, 2, Number{1414213562373095049, -18}}, {Number{2'000'000}, 2, Number{1414213562373095049, -15}}, @@ -1511,7 +1503,7 @@ TEST(NumberTest, to_string) NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); auto const maxMantissa = Number::maxMantissa(); - EXPECT_EQ(maxMantissa, (9'999'999'999'999'999)); + EXPECT_EQ(maxMantissa, 9'999'999'999'999'999); test( Number{false, (maxMantissa * 1000) + 999, -3, Number::Normalized()}, "9999999999999999", @@ -1550,7 +1542,7 @@ TEST(NumberTest, to_string) NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); auto const maxMantissa = Number::maxMantissa(); - EXPECT_EQ((maxMantissa), (9'999'999'999'999'999'999ULL)); + EXPECT_EQ(maxMantissa, 9'999'999'999'999'999'999ULL); test( Number{false, maxMantissa, 0, Number::Normalized{}}, "9999999999999999990", diff --git a/src/tests/libxrpl/basics/base_uint.cpp b/src/tests/libxrpl/basics/base_uint.cpp index 525e7c0407b..9d74f954933 100644 --- a/src/tests/libxrpl/basics/base_uint.cpp +++ b/src/tests/libxrpl/basics/base_uint.cpp @@ -71,9 +71,9 @@ struct BaseUintTest : public ::testing::Test {"fffffffffffffffe", "ffffffffffffffff"}, }); - for (auto const& arg : kTestArgs) + for (auto const& [smallerText, largerText] : kTestArgs) { - xrpl::BaseUInt<64> const u{arg.first}, v{arg.second}; + xrpl::BaseUInt<64> const u{smallerText}, v{largerText}; // For code readability, we want to use general boolean // expectations instead of specific EXPECT_LT etc. EXPECT_TRUE(u < v); @@ -103,9 +103,9 @@ struct BaseUintTest : public ::testing::Test {"fffffffffffffffffffffffe", "ffffffffffffffffffffffff"}, }); - for (auto const& arg : kTestArgs) + for (auto const& [smallerText, largerText] : kTestArgs) { - xrpl::BaseUInt<96> const u{arg.first}, v{arg.second}; + xrpl::BaseUInt<96> const u{smallerText}, v{largerText}; EXPECT_TRUE(u < v); EXPECT_TRUE(u <= v); EXPECT_TRUE(u != v); diff --git a/src/tests/libxrpl/basics/join.cpp b/src/tests/libxrpl/basics/join.cpp index 66c832678b7..427f0b42bcf 100644 --- a/src/tests/libxrpl/basics/join.cpp +++ b/src/tests/libxrpl/basics/join.cpp @@ -19,11 +19,11 @@ struct JoinTest : public ::testing::Test TEST_F(JoinTest, join) { - auto test = [](auto collectionanddelimiter, std::string expected) { + auto test = [](auto collectionAndDelimiter, std::string expected) { std::stringstream ss; // Put something else in the buffer before and after to ensure that // the << operator returns the stream correctly. - ss << "(" << collectionanddelimiter << ")"; + ss << "(" << collectionAndDelimiter << ")"; auto const str = ss.str(); EXPECT_EQ(str.substr(1, str.length() - 2), expected); EXPECT_EQ(str.front(), '('); diff --git a/src/tests/libxrpl/nodestore/Database.cpp b/src/tests/libxrpl/nodestore/Database.cpp index 23087a2f84b..82012ed347e 100644 --- a/src/tests/libxrpl/nodestore/Database.cpp +++ b/src/tests/libxrpl/nodestore/Database.cpp @@ -27,11 +27,11 @@ namespace { std::vector allBackends() { - std::vector types{"memory", "nudb"}; #if XRPL_ROCKSDB_AVAILABLE - types.emplace_back("rocksdb"); + return {"memory", "nudb", "rocksdb"}; +#else + return {"memory", "nudb"}; #endif - return types; } std::vector diff --git a/src/tests/libxrpl/resource/Logic.cpp b/src/tests/libxrpl/resource/Logic.cpp index 383977c896b..e49627ae344 100644 --- a/src/tests/libxrpl/resource/Logic.cpp +++ b/src/tests/libxrpl/resource/Logic.cpp @@ -21,6 +21,7 @@ #include #include #include +#include namespace xrpl::Resource { @@ -72,7 +73,7 @@ class ResourceManagerTest : public ::testing::Test static_cast(v + i), }}; item.address = beast::IP::Endpoint{beast::IP::AddressV4{d}}; - gossip.items.push_back(item); + gossip.items.push_back(std::move(item)); } } }; @@ -88,8 +89,8 @@ TEST_F(ResourceManagerTest, limited_warn_drop) Consumer c{logic.newInboundEndpoint(addr)}; // Create load until we get a warning - int n = 10000; - bool warned = false; + auto n = 10000; + auto warned = false; while (--n >= 0) { @@ -98,7 +99,7 @@ TEST_F(ResourceManagerTest, limited_warn_drop) warned = true; break; } - ++logic.clock(); + logic.advance(); } ASSERT_TRUE(warned) << "Loop count exceeded without warning"; @@ -114,7 +115,7 @@ TEST_F(ResourceManagerTest, limited_warn_drop) EXPECT_TRUE(c.disconnect(j_)); break; } - ++logic.clock(); + logic.advance(); } ASSERT_TRUE(dropped) << "Loop count exceeded without dropping"; @@ -136,7 +137,7 @@ TEST_F(ResourceManagerTest, limited_warn_drop) auto n = kSecondsUntilExpiration + 1s; while (--n > 0s) { - ++logic.clock(); + logic.advance(); logic.periodicActivity(); Consumer const c{logic.newInboundEndpoint(addr)}; if (c.disposition() != Disposition::Drop) @@ -168,7 +169,7 @@ TEST_F(ResourceManagerTest, unlimited_warn_drop) warned = true; break; } - ++logic.clock(); + logic.advance(); } EXPECT_FALSE(warned) << "Should loop forever with no warning"; @@ -238,7 +239,7 @@ TEST_F(ResourceManagerTest, import) 1, }}; item.address = beast::IP::Endpoint{beast::IP::AddressV4{d}}; - g.items.push_back(item); + g.items.push_back(std::move(item)); logic.importConsumers("g", g); } From 95857cdff66c6ee2512acca610fc18f97f3b4b0e Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Wed, 29 Jul 2026 14:59:16 +0100 Subject: [PATCH 3/5] Review comments addressed --- .../libxrpl/nodestore/NodeStoreBench.h | 4 +- src/tests/libxrpl/basics/IntrusiveShared.cpp | 62 ++++++++++++------- src/tests/libxrpl/basics/base_uint.cpp | 60 +++++++++--------- 3 files changed, 70 insertions(+), 56 deletions(-) diff --git a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h index 2b00341fc47..482859361b3 100644 --- a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h +++ b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h @@ -141,7 +141,7 @@ makePool(std::uint8_t prefix, std::size_t count, std::size_t start = 0) Sequence seq(prefix); Batch pool; pool.reserve(count); - for (auto const i : std::views::iota(0uz, count)) + for (auto i = 0uz; i < count; ++i) pool.push_back(seq.obj(start + i)); return pool; } @@ -154,7 +154,7 @@ makeMissingKeys(std::size_t count) Sequence seq(2); std::vector keys; keys.reserve(count); - for (auto const i : std::views::iota(0uz, count)) + for (auto i = 0uz; i < count; ++i) keys.push_back(seq.key(i)); return keys; } diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index a8dd3006d6a..18b6d389a54 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -380,43 +380,57 @@ TEST(IntrusiveSharedTest, partial_delete) std::atomic destructorRan{false}; std::atomic partialDeleteRan{false}; std::latch partialDeleteStartedSyncPoint{2}; + strong->tracingCallback = [&](TrackedState cur, std::optional next) { using enum TrackedState; - if (next == DeletedStarted) - { - // strong goes out of scope while weak is still in scope - // This checks that partialDelete has run to completion - // before the destructor is called. A sleep is inserted - // inside the partial delete to make sure the destructor is - // given an opportunity to run during partial delete. - EXPECT_EQ(cur, PartiallyDeleted); - } - else if (next == PartiallyDeletedStarted) - { - partialDeleteStartedSyncPoint.arrive_and_wait(); - using namespace std::chrono_literals; - // Sleep and let the weak pointer go out of scope, - // potentially triggering a destructor while partial delete - // is running. The test is to make sure that doesn't happen. - std::this_thread::sleep_for(800ms); - } - else if (next == PartiallyDeleted) - { - EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); - } - else if (next == Deleted) + if (!next) + return; + + switch (*next) { - EXPECT_FALSE(destructorRan.exchange(true)); + case DeletedStarted: + // strong goes out of scope while weak is still in scope + // This checks that partialDelete has run to completion + // before the destructor is called. A sleep is inserted + // inside the partial delete to make sure the destructor is + // given an opportunity to run during partial delete. + EXPECT_EQ(cur, PartiallyDeleted); + break; + + case PartiallyDeletedStarted: { + partialDeleteStartedSyncPoint.arrive_and_wait(); + using namespace std::chrono_literals; + // Sleep and let the weak pointer go out of scope, + // potentially triggering a destructor while partial delete + // is running. The test is to make sure that doesn't happen. + std::this_thread::sleep_for(800ms); + break; + } + + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); + break; + + case Deleted: + EXPECT_FALSE(destructorRan.exchange(true)); + break; + + case Uninitialized: + case Alive: + break; } }; + std::thread t1{[&] { partialDeleteStartedSyncPoint.arrive_and_wait(); weak.reset(); // Trigger a full delete as soon as the partial // delete starts }}; + std::thread t2{[&] { strong.reset(); // Trigger a partial delete }}; + t1.join(); t2.join(); diff --git a/src/tests/libxrpl/basics/base_uint.cpp b/src/tests/libxrpl/basics/base_uint.cpp index 9d74f954933..10795f4563f 100644 --- a/src/tests/libxrpl/basics/base_uint.cpp +++ b/src/tests/libxrpl/basics/base_uint.cpp @@ -73,23 +73,23 @@ struct BaseUintTest : public ::testing::Test for (auto const& [smallerText, largerText] : kTestArgs) { - xrpl::BaseUInt<64> const u{smallerText}, v{largerText}; + xrpl::BaseUInt<64> const smaller{smallerText}, larger{largerText}; // For code readability, we want to use general boolean // expectations instead of specific EXPECT_LT etc. - EXPECT_TRUE(u < v); - EXPECT_TRUE(u <= v); - EXPECT_TRUE(u != v); - EXPECT_FALSE(u == v); - EXPECT_FALSE(u > v); - EXPECT_FALSE(u >= v); - EXPECT_FALSE(v < u); - EXPECT_FALSE(v <= u); - EXPECT_TRUE(v != u); - EXPECT_FALSE(v == u); - EXPECT_TRUE(v > u); - EXPECT_TRUE(v >= u); - EXPECT_TRUE(u == u); - EXPECT_TRUE(v == v); + EXPECT_TRUE(smaller < larger); + EXPECT_TRUE(smaller <= larger); + EXPECT_TRUE(smaller != larger); + EXPECT_FALSE(smaller == larger); + EXPECT_FALSE(smaller > larger); + EXPECT_FALSE(smaller >= larger); + EXPECT_FALSE(larger < smaller); + EXPECT_FALSE(larger <= smaller); + EXPECT_TRUE(larger != smaller); + EXPECT_FALSE(larger == smaller); + EXPECT_TRUE(larger > smaller); + EXPECT_TRUE(larger >= smaller); + EXPECT_TRUE(smaller == smaller); + EXPECT_TRUE(larger == larger); } } @@ -105,21 +105,21 @@ struct BaseUintTest : public ::testing::Test for (auto const& [smallerText, largerText] : kTestArgs) { - xrpl::BaseUInt<96> const u{smallerText}, v{largerText}; - EXPECT_TRUE(u < v); - EXPECT_TRUE(u <= v); - EXPECT_TRUE(u != v); - EXPECT_FALSE(u == v); - EXPECT_FALSE(u > v); - EXPECT_FALSE(u >= v); - EXPECT_FALSE(v < u); - EXPECT_FALSE(v <= u); - EXPECT_TRUE(v != u); - EXPECT_FALSE(v == u); - EXPECT_TRUE(v > u); - EXPECT_TRUE(v >= u); - EXPECT_TRUE(u == u); - EXPECT_TRUE(v == v); + xrpl::BaseUInt<96> const smaller{smallerText}, larger{largerText}; + EXPECT_TRUE(smaller < larger); + EXPECT_TRUE(smaller <= larger); + EXPECT_TRUE(smaller != larger); + EXPECT_FALSE(smaller == larger); + EXPECT_FALSE(smaller > larger); + EXPECT_FALSE(smaller >= larger); + EXPECT_FALSE(larger < smaller); + EXPECT_FALSE(larger <= smaller); + EXPECT_TRUE(larger != smaller); + EXPECT_FALSE(larger == smaller); + EXPECT_TRUE(larger > smaller); + EXPECT_TRUE(larger >= smaller); + EXPECT_TRUE(smaller == smaller); + EXPECT_TRUE(larger == larger); } } } From 9970d7bd1bba62654a8b6e35c477bf73a50db11b Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Wed, 29 Jul 2026 16:11:44 +0100 Subject: [PATCH 4/5] More switch --- src/tests/libxrpl/basics/IntrusiveShared.cpp | 104 +++++++++++++------ 1 file changed, 74 insertions(+), 30 deletions(-) diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index 18b6d389a54..b9f8930b7b1 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -458,13 +458,24 @@ TEST(IntrusiveSharedTest, destructor) std::latch weakResetSyncPoint{2}; strong->tracingCallback = [&](TrackedState cur, std::optional next) { using enum TrackedState; - if (next == PartiallyDeleted) - { - EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); - } - else if (next == Deleted) + if (!next) + return; + + switch (*next) { - EXPECT_FALSE(destructorRan.exchange(true)); + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); + break; + + case Deleted: + EXPECT_FALSE(destructorRan.exchange(true)); + break; + + case Uninitialized: + case Alive: + case PartiallyDeletedStarted: + case DeletedStarted: + break; } }; std::thread t1{[&] { @@ -506,15 +517,26 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_variant) auto tracingCallback = [&](TrackedState cur, std::optional next) { using enum TrackedState; auto [destructorRan, partialDeleteRan] = getDestructorState(); - if (next == PartiallyDeleted) - { - EXPECT_FALSE(partialDeleteRan || destructorRan); - setPartialDeleteRan(); - } - else if (next == Deleted) + if (!next) + return; + + switch (*next) { - EXPECT_FALSE(destructorRan); - setDestructorRan(); + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan || destructorRan); + setPartialDeleteRan(); + break; + + case Deleted: + EXPECT_FALSE(destructorRan); + setDestructorRan(); + break; + + case Uninitialized: + case Alive: + case PartiallyDeletedStarted: + case DeletedStarted: + break; } }; auto createVecOfPointers = [&](auto const& toClone, std::default_random_engine& eng) @@ -637,15 +659,26 @@ TEST(IntrusiveSharedTest, multithreaded_clear_mixed_union) auto tracingCallback = [&](TrackedState cur, std::optional next) { using enum TrackedState; auto [destructorRan, partialDeleteRan] = getDestructorState(); - if (next == PartiallyDeleted) - { - EXPECT_FALSE(partialDeleteRan || destructorRan); - setPartialDeleteRan(); - } - else if (next == Deleted) + if (!next) + return; + + switch (*next) { - EXPECT_FALSE(destructorRan); - setDestructorRan(); + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan || destructorRan); + setPartialDeleteRan(); + break; + + case Deleted: + EXPECT_FALSE(destructorRan); + setDestructorRan(); + break; + + case Uninitialized: + case Alive: + case PartiallyDeletedStarted: + case DeletedStarted: + break; } }; auto createVecOfPointers = @@ -775,15 +808,26 @@ TEST(IntrusiveSharedTest, multithreaded_locking_weak) auto tracingCallback = [&](TrackedState cur, std::optional next) { using enum TrackedState; auto [destructorRan, partialDeleteRan] = getDestructorState(); - if (next == PartiallyDeleted) - { - EXPECT_FALSE(partialDeleteRan || destructorRan); - setPartialDeleteRan(); - } - else if (next == Deleted) + if (!next) + return; + + switch (*next) { - EXPECT_FALSE(destructorRan); - setDestructorRan(); + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan || destructorRan); + setPartialDeleteRan(); + break; + + case Deleted: + EXPECT_FALSE(destructorRan); + setDestructorRan(); + break; + + case Uninitialized: + case Alive: + case PartiallyDeletedStarted: + case DeletedStarted: + break; } }; From 9f7a288e578e7e6867e777a7dc832b7fdc53922c Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Thu, 30 Jul 2026 17:38:51 +0100 Subject: [PATCH 5/5] Address review comments --- src/benchmarks/libxrpl/nodestore/Backend.cpp | 2 +- .../libxrpl/nodestore/NodeStoreBench.h | 2 +- src/tests/libxrpl/resource/Logic.cpp | 20 +++++++++---------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/benchmarks/libxrpl/nodestore/Backend.cpp b/src/benchmarks/libxrpl/nodestore/Backend.cpp index c739013b88d..cd3e15bd651 100644 --- a/src/benchmarks/libxrpl/nodestore/Backend.cpp +++ b/src/benchmarks/libxrpl/nodestore/Backend.cpp @@ -290,7 +290,7 @@ registerStoreBatch(BackendConfig const& bc) rs->harness = std::make_unique(cfg); rs->present = makePool(1, poolSize); rs->avgPayload = averagePayload(rs->present); - std::vector const batches = slicePreciseBatches(rs->present, kBatchSize); + std::vector const batches = sliceFixedBatches(rs->present, kBatchSize); if (batches.empty()) { state.SkipWithError("pool smaller than one batch"); diff --git a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h index 482859361b3..debdc5d47a6 100644 --- a/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h +++ b/src/benchmarks/libxrpl/nodestore/NodeStoreBench.h @@ -211,7 +211,7 @@ makeShuffle(std::size_t size, std::uint64_t seed) // Partition a pool into fixed-size batches. Any trailing remainder shorter than // `batchSize` is dropped, so every returned batch has exactly `batchSize`. inline std::vector -slicePreciseBatches(Batch const& pool, std::size_t batchSize) +sliceFixedBatches(Batch const& pool, std::size_t batchSize) { std::vector batches; if (batchSize == 0) diff --git a/src/tests/libxrpl/resource/Logic.cpp b/src/tests/libxrpl/resource/Logic.cpp index e49627ae344..a3362b45409 100644 --- a/src/tests/libxrpl/resource/Logic.cpp +++ b/src/tests/libxrpl/resource/Logic.cpp @@ -17,9 +17,10 @@ #include #include -#include +#include #include #include +#include #include #include @@ -56,9 +57,10 @@ class ResourceManagerTest : public ::testing::Test //-------------------------------------------------------------------------- - static void - populateGossip(Gossip& gossip) + static Gossip + makeGossip() { + Gossip gossip; std::uint8_t const v(10 + randInt(9)); std::uint8_t const n(10 + randInt(9)); gossip.items.reserve(n); @@ -75,6 +77,7 @@ class ResourceManagerTest : public ::testing::Test item.address = beast::IP::Endpoint{beast::IP::AddressV4{d}}; gossip.items.push_back(std::move(item)); } + return gossip; } }; @@ -215,14 +218,9 @@ TEST_F(ResourceManagerTest, imports) TestLogic logic{j_}; static constexpr auto kGossipSources = 5uz; - - std::array gossip; - - for (auto& g : gossip) - populateGossip(g); - - for (auto i = 0uz; i < gossip.size(); ++i) - logic.importConsumers(std::to_string(i), gossip[i]); + std::ranges::for_each(std::views::iota(0uz, kGossipSources), [&](auto const i) { + logic.importConsumers(std::to_string(i), makeGossip()); + }); } TEST_F(ResourceManagerTest, import)