chore: Gtest migration followups first pass by godexsoft · Pull Request #7884 · XRPLF/rippled

godexsoft · 2026-07-28T14:32:51Z

High Level Overview of Change

Follow-up PR that resolves review comments left open on seven already-merged
test-migration PRs: #7136, #7133, #7054, #7046, #7317, #7304, #7292.

Those PRs were merged with a number of "let's push this to the next PR" / "keep this
convo open" threads. This audits all 72 unresolved threads across them and clears the
ones that are mechanical, plus one real bug.

39 review threads can be resolved from this PR — 33 fixed in code, 6 answered
without a code change (see Deliberately not changed below).

Test, benchmark and build files only. No production code is touched.

Context of Change

The bulk is grouped into four mechanical sweeps rather than 15 scattered follow-ups,
because the same feedback recurred across PRs:

Theme	What	Threads
`std::size_t` loop counters	40 of 48 signed counters under `src/tests/libxrpl` + `src/benchmarks`	7136.14, 7133.14, 7317.19
Magic numbers	extracted to named `constexpr` in `resource/Logic.cpp`, `shamap/SHAMap.cpp`, `shamap/SHAMapSync.cpp`, `benchmarks/Backend.cpp`	7133.5, 7133.16, 7133.17, 7133.18, 7317.15
`std::to_array`	replaced sized `std::array<…, N>{{…}}` and raw C arrays	7136.6, 7136.7
`ranges`	`ranges::iota`, `ranges::shuffle`, `views::iota` in the benchmark helpers	7317.21, 7317.23

Bug fix: `randomWeightedShuffle`

#7046 thread — flagged
in review, deferred as out of scope, never picked up. src/tests/libxrpl/csf/random.h:

std::discrete_distribution<> dd(w.begin() + i, w.end());
auto idx = dd(g);          // relative to w.begin() + i, used as an absolute index
std::swap(v[i], v[idx]);

dd is constructed from w.begin() + i, so its result is relative to i and has to be
offset back before indexing. As written, for i > 0 it swaps against the wrong prefix
element, so the result is not the weighted-without-replacement shuffle it claims to be.
Two further defects on the same two lines: v.size() - 1 underflows to SIZE_MAX on an
empty vector, and int i was compared against size_t.

This is consumed by csf/PeerGroup.h, so it silently skewed peer selection in consensus
simulations. This is the only behavioural change in the PR; the consensus and csf suites
still pass.

Other fixes

test: Migrate basics Beast tests to GTest #7136 — structured binding over the test pairs; using swu → using SharedWeak;
if → else if on five mutually-exclusive next == chains; collectionanddelimiter
→ collectionAndDelimiter; comments now name kMaxRep correctly; dropped gratuitous
parens in EXPECT_EQ; deleted a commented-out block referencing
MantissaRange::mantissa_scale::small, an enum that no longer exists, so the block
could never have been un-commented as written.
test: Migrate resource, shamap Beast tests to GTest #7133 — ++logic.clock() → logic.advance() (4 sites; TestLogic::advance() is
literally ++member, so this is identical); push_back(std::move(item)) for
Gossip::Item; auto for n / warned.
test: Add google benchmark dependency and migrate nodestore timing test as a benchmark #7317 — include_guard() in cmake/XrplAddBenchmark.cmake; sliceBatches →
slicePreciseBatches; rngcpy unified into one while (bytes > 0) loop using
std::min; auto& → auto const& on the destructured IterateContext (5 sites);
RunState members documented with ///<, including harness which had none;
"member order matters" note moved onto tempDir itself.
test: Migrate nodestore tests from Beast to GTest #7292 — allBackends() now returns directly per #if branch so nothing is ever
mutated. This removes the const question entirely rather than suppressing it with a
NOLINT, which is what the thread asked for.

API Impact

No impact.

codecov · 2026-07-28T15:01:04Z

Codecov Report

❌ Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/benchmarks/libxrpl/nodestore/Backend.cpp	0.0%	10 Missing ⚠️
src/benchmarks/libxrpl/nodestore/NodeStoreBench.h	0.0%	10 Missing ⚠️

📢 Thoughts on this report? Let us know!

mathbunnyru

I'm not a fan of auto i = 0uz, in my opinion size_t i = 0 is as short, but doesn't make you remember what uz is, and the type is evident

mathbunnyru · 2026-07-28T20:11:33Z

    std::vector<std::uint64_t> coeffs;
    coeffs.reserve(numCoeff);
-    for (int i = 0; i < numCoeff; ++i)
+    for (auto i = 0uz; i < numCoeff; ++i)


Suggested change

for (auto i = 0uz; i < numCoeff; ++i)

for (size_t i = 0; i < numCoeff; ++i)

Can't we use this simple code?

And later too

I prefer auto and proper C++23 with uz

I don't understand how auto is helpful here at all, if it makes the code less obvious

you need to remember what uz means (I don't for example)

it doesn't make code shorter

makes it less expressive, because we now don't specify the type

size_t i = 0; is perfectly proper C++23. Just because it worked before, doesn't make it bad 🙂

godexsoft · 2026-07-29T13:52:54Z

I'm not a fan of auto i = 0uz, in my opinion size_t i = 0 is as short, but doesn't make you remember what uz is, and the type is evident

I'm totally on the opposite side in regards to that :) i don't like to spell out types everywhere and uz is a standard way to use auto in such places.

mathbunnyru · 2026-07-29T14:17:30Z

            EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load());
        }
-        if (next == Deleted)
+        else if (next == Deleted)


Please, make it switch here too

mathbunnyru · 2026-07-29T14:26:26Z

    std::vector<std::uint64_t> coeffs;
    coeffs.reserve(numCoeff);
-    for (int i = 0; i < numCoeff; ++i)
+    for (auto i = 0uz; i < numCoeff; ++i)


I don't understand how auto is helpful here at all, if it makes the code less obvious

you need to remember what uz means (I don't for example)

it doesn't make code shorter

makes it less expressive, because we now don't specify the type

size_t i = 0; is perfectly proper C++23. Just because it worked before, doesn't make it bad 🙂

mathbunnyru · 2026-07-29T14:28:24Z

-        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)


Btw, you can probably just make it const.

Re uz: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#nl11-make-literals-readable
You don't need to understand or like it, it's just one way to write this and i prefer it :)

Re const: can't make it const because dd() below will not compile unfortunately :(

mathbunnyru

I disagree with usage of the auto in following cases, but since we don't have a style guide for this, approving

auto i = 0uz;
auto whether = true;
auto arr = std::to_array<type>({some_values});

Copilot

Pull request overview

This PR applies follow-up cleanups from prior GTest-migration work across the test and benchmark suites, and includes a correctness fix in CSF’s randomWeightedShuffle used by consensus simulations.

Changes:

Fix randomWeightedShuffle to correctly offset std::discrete_distribution results and avoid empty-vector underflow.
Mechanical test/benchmark cleanup: migrate many loop counters to std::size_t/0uz, extract/rename magic numbers, and simplify some assertions/iteration logic.
Benchmark helper refinements: adopt std::ranges algorithms, tighten const-correctness in structured bindings, and clarify helper naming/docs.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/tests/libxrpl/shamap/SHAMapSync.cpp	Replaces magic numbers with named constants and uses `std::size_t` loop counters.
src/tests/libxrpl/shamap/SHAMap.cpp	Simplifies traversal assertion logic and names proof-path key bounds.
src/tests/libxrpl/resource/Logic.cpp	Minor test cleanups: move-into containers, use `advance()`, replace raw arrays with `std::array`, and update loop counters.
src/tests/libxrpl/nodestore/Database.cpp	Simplifies backend-list construction to avoid mutation under `#if` branches.
src/tests/libxrpl/csf/TrustGraph.h	Converts nested loops to `std::size_t`-friendly counters.
src/tests/libxrpl/csf/random.h	Fixes weighted shuffle indexing/underflow and updates loop counter types.
src/tests/libxrpl/consensus/CensorshipDetector.cpp	Converts loop counter to `std::size_t` literal-suffixed form.
src/tests/libxrpl/basics/Number.cpp	Comment/cleanup adjustments and removes dead commented-out code.
src/tests/libxrpl/basics/MallocTrim.cpp	Converts loop counter to `std::size_t` literal-suffixed form.
src/tests/libxrpl/basics/join.cpp	Renames lambda parameter for clarity/consistency.
src/tests/libxrpl/basics/IntrusiveShared.cpp	Converts counters/types to `std::size_t` and refactors optional-state handling to a `switch`.
src/tests/libxrpl/basics/base58.cpp	Converts multiple loop counters to `std::size_t` literal-suffixed form.
src/tests/libxrpl/basics/base_uint.cpp	Uses `std::to_array` and structured bindings for more readable constexpr test vectors.
src/benchmarks/libxrpl/nodestore/NodeStoreBench.h	Uses `std::ranges` algorithms, simplifies RNG buffer fill, and renames batch slicing helper.
src/benchmarks/libxrpl/nodestore/Backend.cpp	Replaces magic numbers with constants, improves const-correctness, and updates helper name usage.
cmake/XrplAddBenchmark.cmake	Adds `include_guard()` to prevent multiple inclusion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bthomee · 2026-07-29T23:54:15Z

 // `batchSize` is dropped, so every returned batch has exactly `batchSize`.
 inline std::vector<Batch>
-sliceBatches(Batch const& pool, std::size_t batchSize)
+slicePreciseBatches(Batch const& pool, std::size_t batchSize)


What's the benefit of this renaming? If renaming is necessary, wouldn't sliceFixedBatches be more appropriate?

Because that was suggested by @mathbunnyru : #7317 (comment)

sliceFixedBatches is a great name, let's use that

bthomee · 2026-07-30T00:03:13Z

+    static constexpr auto kGossipSources = 5uz;
+
+    std::array<Gossip, kGossipSources> 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]);


Is there a more elegant way to write this? It looks quite convoluted.

Yea with ranges and enumerate_view i guess. I can try. It is supposed to be in c++23 so let's hope all compilers can do it by now

Unfortunately enumerate_view is not yet shipped by libc++ so i can't think of a better loop than the one we already have here.

for (auto const i : std::views::iota(0uz, gossip.size())) logic.importConsumers(std::to_string(i), gossip[i]);

does not look much better to me.

Bummer, thanks for trying!

I have found a simpler way that works fine and since this is in tests i think it's better.

godexsoft added 2 commits July 28, 2026 13:58

Simplest fixes

9dd8f3e

More small fixes

6e71c03

godexsoft requested a review from mathbunnyru July 28, 2026 19:01

mathbunnyru reviewed Jul 28, 2026

View reviewed changes

Review comments addressed

95857cd

godexsoft requested a review from mathbunnyru July 29, 2026 14:14

mathbunnyru reviewed Jul 29, 2026

View reviewed changes

More switch

9970d7b

mathbunnyru approved these changes Jul 29, 2026

View reviewed changes

bthomee requested a review from Copilot July 29, 2026 23:44

Copilot started reviewing on behalf of bthomee July 29, 2026 23:45 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

bthomee reviewed Jul 30, 2026

View reviewed changes

godexsoft requested a review from bthomee July 30, 2026 15:40

bthomee approved these changes Jul 30, 2026

View reviewed changes

Address review comments

9f7a288

	for (auto i = 0uz; i < numCoeff; ++i)
	for (size_t i = 0; i < numCoeff; ++i)

Uh oh!

Conversation

godexsoft commented Jul 28, 2026

High Level Overview of Change

Context of Change

Bug fix: randomWeightedShuffle

Other fixes

API Impact

Uh oh!

codecov Bot commented Jul 28, 2026

Codecov Report

Uh oh!

mathbunnyru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

godexsoft commented Jul 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathbunnyru left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bug fix: `randomWeightedShuffle`