chore: Enable remaining clang-tidy `performance` checks by godexsoft · Pull Request #6648 · XRPLF/rippled

godexsoft · 2026-03-25T14:09:23Z

High Level Overview of Change

This PR enables the following clang-tidy checks:

performance-faster-string-find
performance-for-range-copy
performance-inefficient-vector-operation
performance-move-const-arg
performance-no-automatic-move

API Impact

No impact.

godexsoft · 2026-03-25T15:49:01Z

src/test/protocol/MultiApiJson_test.cpp

                        [](auto...) {});
                };
-            }(std::move(std::as_const(s1))));
+            }(std::move(std::as_const(s1))));  // NOLINT(performance-move-const-arg)


I believe these are deliberately doing the "wrong" thing here

s1 is a MultiApiJson which contains only one std::array so it is trivially copyable. I think clang-tidy is right here and in the other similar places where you've added nolint

codecov · 2026-03-25T17:02:58Z

Codecov Report

❌ Patch coverage is 89.65517% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.5%. Comparing base (509677a) to head (6c0c8ad).

Files with missing lines	Patch %	Lines
src/xrpld/app/consensus/RCLCxPeerPos.cpp	0.0%	2 Missing ⚠️
src/xrpld/app/main/GRPCServer.cpp	33.3%	2 Missing ⚠️
src/xrpld/app/misc/detail/ValidatorList.cpp	66.7%	1 Missing ⚠️
src/xrpld/rpc/detail/RPCCall.cpp	66.7%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6648   +/-   ##
=======================================
  Coverage     81.4%   81.5%           
=======================================
  Files          998     998           
  Lines        74456   74454    -2     
  Branches      7574    7556   -18     
=======================================
+ Hits         60624   60643   +19     
+ Misses       13832   13811   -21

Files with missing lines	Coverage Δ
src/libxrpl/basics/FileUtilities.cpp	`69.2% <ø> (ø)`
src/libxrpl/ledger/Ledger.cpp	`82.1% <100.0%> (-<0.1%)`	⬇️
src/libxrpl/ledger/helpers/TokenHelpers.cpp	`93.5% <100.0%> (ø)`
src/libxrpl/protocol/BuildInfo.cpp	`98.4% <100.0%> (ø)`
src/libxrpl/protocol/STPathSet.cpp	`94.1% <100.0%> (ø)`
src/libxrpl/shamap/SHAMap.cpp	`91.4% <100.0%> (+0.4%)`	⬆️
...xrpl/tx/invariants/PermissionedDomainInvariant.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/tx/invariants/VaultInvariant.cpp	`98.2% <100.0%> (ø)`
src/libxrpl/tx/transactors/bridge/XChainBridge.cpp	`91.3% <100.0%> (ø)`
.../libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp	`94.9% <100.0%> (ø)`
... and 28 more

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-03-26T00:44:37Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

src/xrpld/rpc/detail/RPCCall.cpp

github-actions · 2026-03-27T01:14:44Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

kuznetsss · 2026-03-27T12:07:03Z

src/libxrpl/shamap/SHAMap.cpp

                        // Right now no move will occur; With c++-20 child will
                        // be moved from.
-                        node = intr_ptr::static_pointer_cast<SHAMapInnerNode>(std::move(child));
+                        node = intr_ptr::static_pointer_cast<SHAMapInnerNode>(child);


Probably not related, but what is the comment here about if all the nodes are operated by pointers?

kuznetsss · 2026-03-27T12:09:06Z

src/test/app/Invariants_test.cpp

                auto nftPage = std::make_shared<SLE>(keylet::nftpage(
                    keylet::nftpage_max(A1), (nfTokens[1].getFieldH256(sfNFTokenID))));
-                nftPage->setFieldArray(sfNFTokens, std::move(nfTokens));
+                nftPage->setFieldArray(sfNFTokens, nfTokens);


We remove std::move() here and in the other places because the object is trivially copyable, right?

kuznetsss · 2026-03-27T12:15:41Z

src/test/protocol/MultiApiJson_test.cpp

                        [](auto...) {});
                };
-            }(std::move(std::as_const(s1))));
+            }(std::move(std::as_const(s1))));  // NOLINT(performance-move-const-arg)


s1 is a MultiApiJson which contains only one std::array so it is trivially copyable. I think clang-tidy is right here and in the other similar places where you've added nolint

Copilot

Pull request overview

This PR enables several previously-disabled clang-tidy performance-* checks and updates the codebase to satisfy the new diagnostics, largely by removing ineffective std::move calls, avoiding unnecessary copies in range-for loops, and reserving vector capacity where appropriate.

Changes:

Enable additional clang-tidy performance checks in .clang-tidy.
Mechanical refactors across RPC/overlay/app/lib/test code to comply with the new checks (string find char overloads, range-for by const&, remove ineffective moves, add reserve()).
A few small signature/initializer updates to avoid “move-const-arg”/“no-automatic-move” warnings.

Reviewed changes

Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/xrpld/rpc/handlers/ServerDefinitions.cpp`	Use `find(char)` overload for faster string search.
`src/xrpld/rpc/handlers/NoRippleCheck.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/LedgerEntry.cpp`	Remove ineffective `std::move` when passing sorted credentials.
`src/xrpld/rpc/handlers/GatewayBalances.cpp`	Remove ineffective moves; avoid extra copies in inserts where possible.
`src/xrpld/rpc/handlers/DepositAuthorized.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AccountTx.cpp`	Allow move/avoid const-inhibiting patterns for status locals.
`src/xrpld/rpc/handlers/AccountOffers.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AccountLines.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AccountInfo.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AccountCurrenciesHandler.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AccountChannels.cpp`	Remove ineffective `std::move` from optional value extraction.
`src/xrpld/rpc/handlers/AMMInfo.cpp`	Remove ineffective `std::move` of `const` shared_ptr-like value.
`src/xrpld/rpc/detail/ServerHandler.cpp`	Use `find(char)` for header parsing.
`src/xrpld/rpc/detail/RPCCall.cpp`	Use `find_last_of(char)`; remove ineffective move into `Json::Value`; add clarifying comments.
`src/xrpld/overlay/detail/OverlayImpl.cpp`	Remove ineffective move from `const&`; range-for by `const&`.
`src/xrpld/app/rdb/backend/detail/SQLiteDatabase.cpp`	Avoid moving blobs when only read-only access is needed.
`src/xrpld/app/misc/detail/ValidatorSite.cpp`	Avoid moving response when downstream takes `const&`.
`src/xrpld/app/misc/detail/ValidatorList.cpp`	Range-for by `const&`; avoid const-inhibiting locals.
`src/xrpld/app/misc/NetworkOPs.cpp`	Adjust `TxQ::Metrics` parameter passing and initialization style.
`src/xrpld/app/main/Main.cpp`	Add `reserve()` before filling child-process vector.
`src/xrpld/app/main/GRPCServer.cpp`	Use `find_first/last_of(char)`; remove ineffective move for small enum-like field.
`src/xrpld/app/main/Application.cpp`	Range-for by `const&` for startup RPC commands.
`src/xrpld/app/ledger/detail/LedgerMaster.cpp`	Remove ineffective move into `const&` parameter.
`src/xrpld/app/consensus/RCLValidations.cpp`	Remove ineffective move into `const&` parameter.
`src/xrpld/app/consensus/RCLCxPeerPos.h`	Change proposal parameter passing to satisfy performance checks.
`src/xrpld/app/consensus/RCLCxPeerPos.cpp`	Match constructor signature/initialization to header.
`src/test/protocol/MultiApiJson_test.cpp`	Add `NOLINT` for intentional move-const-arg usage in static-assert tests.
`src/test/ledger/Directory_test.cpp`	Range-for by `const&`.
`src/test/jtx/impl/mpt.cpp`	Range-for by `const&`.
`src/test/jtx/impl/AMMTest.cpp`	Avoid const local that could inhibit moves/copies per tidy.
`src/test/consensus/Validations_test.cpp`	Range-for by `const&`; add `reserve()` for expected fee vector.
`src/test/conditions/PreimageSha256_test.cpp`	Range-for by `const&`.
`src/test/beast/aged_associative_container_test.cpp`	Range-for by `const&`.
`src/test/basics/IntrusiveShared_test.cpp`	Add `reserve()` for thread vectors.
`src/test/app/Vault_test.cpp`	Avoid const local that could inhibit moves/copies per tidy.
`src/test/app/ValidatorSite_test.cpp`	Add `reserve()` for URI collection.
`src/test/app/ValidatorList_test.cpp`	Add `reserve()` for publisher key strings.
`src/test/app/Transaction_ordering_test.cpp`	Add `reserve()` and replace magic number with constant.
`src/test/app/RCLValidations_test.cpp`	Range-for by `const&`.
`src/test/app/Offer_test.cpp`	Range-for by `const&`.
`src/test/app/Manifest_test.cpp`	Remove ineffective `std::move` into `const&` overload.
`src/test/app/Loan_test.cpp`	Avoid copies in lambda returns; add `reserve()` for broker vector.
`src/test/app/LedgerReplay_test.cpp`	Add `reserve()` for skip-list vector.
`src/test/app/Invariants_test.cpp`	Remove ineffective moves into `const&` APIs.
`src/test/app/Batch_test.cpp`	Add `reserve()` for txn id strings.
`src/libxrpl/tx/transactors/oracle/OracleSet.cpp`	Remove ineffective moves from `const&` elements.
`src/libxrpl/tx/transactors/nft/NFTokenUtils.cpp`	Remove ineffective move into `const&` overload.
`src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp`	Remove ineffective move into `const&` overload.
`src/libxrpl/tx/transactors/bridge/XChainBridge.cpp`	Avoid const local that could inhibit moves/copies per tidy.
`src/libxrpl/tx/invariants/VaultInvariant.cpp`	Remove ineffective `std::move` in returns.
`src/libxrpl/tx/invariants/PermissionedDomainInvariant.cpp`	Remove ineffective move of trivially-copyable struct.
`src/libxrpl/shamap/SHAMap.cpp`	Remove move of `child` and rely on consistent behavior.
`src/libxrpl/protocol/STPathSet.cpp`	Range-for by `const&`.
`src/libxrpl/protocol/BuildInfo.cpp`	Range-for by `const&`.
`src/libxrpl/ledger/helpers/TokenHelpers.cpp`	Avoid const local that could inhibit moves/copies per tidy.
`src/libxrpl/ledger/Ledger.cpp`	Remove ineffective move/copy patterns; range-for by `const&`.
`src/libxrpl/basics/FileUtilities.cpp`	Remove const local to preserve move behavior on return.
`.clang-tidy`	Enable remaining clang-tidy performance checks.

Comments suppressed due to low confidence (1)

src/xrpld/app/misc/NetworkOPs.cpp:207

TxQ::Metrics is not trivially copyable (it contains std::optional), so the inline comment is misleading. Since the ctor takes escalationMetrics by value, initializing em from escalationMetrics copies; consider moving it into the std::optional to avoid an extra copy when the caller provides a temporary.

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

Copilot · 2026-03-27T14:08:31Z

src/libxrpl/shamap/SHAMap.cpp

                        // The semantics of this changes when we move to c++-20
                        // Right now no move will occur; With c++-20 child will
                        // be moved from.


The comment about move semantics changing "when we move to c++-20" is now misleading: the code no longer uses std::move(child) here, so the described behavior doesn’t apply. Please update or remove the comment so it matches the current code and C++20 build settings.

Suggested change

// The semantics of this changes when we move to c++-20

// Right now no move will occur; With c++-20 child will

// be moved from.

Copilot · 2026-03-27T14:08:32Z

src/xrpld/app/consensus/RCLCxPeerPos.h

    RCLCxPeerPos(
        PublicKey const& publicKey,
        Slice const& signature,
        uint256 const& suppress,
-        Proposal&& proposal);
+        Proposal const& proposal);  // trivially copyable


The constructor now takes Proposal const& and the comment says "trivially copyable", but ConsensusProposal is not trivially copyable (it contains e.g. std::optional). Also, taking by const-ref forces a copy when callers pass a temporary (see PeerImp.cpp constructing a Proposal{...} inline). Consider taking Proposal by value and moving into proposal_, or providing both const&/&& overloads, and remove/adjust the "trivially copyable" comment to match reality.

godexsoft added 3 commits March 25, 2026 02:56

Autofixes for new checks

21e817c

Autofixes and manual additions

2ff60db

Merge branch 'develop' into chore/clang-tidy-checks-performance

6c71288

godexsoft added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Mar 25, 2026

godexsoft commented Mar 25, 2026

View reviewed changes

godexsoft added 2 commits March 25, 2026 16:13

Fix some places

7a9c057

Fix one more instance found in CI

8f59ba0

godexsoft removed the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Mar 25, 2026

godexsoft marked this pull request as ready for review March 25, 2026 19:27

github-actions bot added the PR: has conflicts label Mar 26, 2026

bthomee approved these changes Mar 26, 2026

View reviewed changes

src/xrpld/rpc/detail/RPCCall.cpp Show resolved Hide resolved

godexsoft added 2 commits March 26, 2026 23:01

Reference issue for removed move

60d9d31

Merge branch 'develop' into chore/clang-tidy-checks-performance

6c0c8ad

github-actions bot removed the PR: has conflicts label Mar 27, 2026

godexsoft requested a review from kuznetsss March 27, 2026 04:34

kuznetsss reviewed Mar 27, 2026

View reviewed changes

kuznetsss requested a review from Copilot March 27, 2026 14:01

Copilot started reviewing on behalf of kuznetsss March 27, 2026 14:02 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Enable remaining clang-tidy `performance` checks#6648

chore: Enable remaining clang-tidy `performance` checks#6648
godexsoft wants to merge 7 commits intoXRPLF:developfrom
godexsoft:chore/clang-tidy-checks-performance

godexsoft commented Mar 25, 2026 •

edited

Loading

Uh oh!

godexsoft Mar 25, 2026

Uh oh!

kuznetsss Mar 27, 2026

Uh oh!

codecov bot commented Mar 25, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Uh oh!

github-actions bot commented Mar 27, 2026

Uh oh!

kuznetsss Mar 27, 2026

Uh oh!

kuznetsss Mar 27, 2026

Uh oh!

kuznetsss Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// The semantics of this changes when we move to c++-20
	// Right now no move will occur; With c++-20 child will
	// be moved from.

Conversation

godexsoft commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

API Impact

Uh oh!

godexsoft Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 26, 2026

Uh oh!

Uh oh!

github-actions bot commented Mar 27, 2026

Uh oh!

kuznetsss Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

kuznetsss Mar 27, 2026

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!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

godexsoft commented Mar 25, 2026 •

edited

Loading

codecov bot commented Mar 25, 2026 •

edited

Loading