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

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

                        [](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

When i see an explicit std::move of std::as_const i suspect that this is deliberately added for tests here, possibly to test binding (often done using static_assert). clang-tidy is 'right' but that also means we want to keep this code.

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

Codecov Report

❌ Patch coverage is 91.17647% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (3a477e4) to head (6001e41).
⚠️ Report is 1 commits behind head on develop.

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.4%   -0.0%     
=========================================
  Files          999     999             
  Lines        74463   74458      -5     
  Branches      7560    7556      -4     
=========================================
- Hits         60650   60644      -6     
- Misses       13813   13814      +1

Files with missing lines	Coverage Δ
include/xrpl/protocol/STVector256.h	`100.0% <ø> (ø)`
include/xrpl/tx/transactors/dex/AMMHelpers.h	`96.0% <100.0%> (-<0.1%)`	⬇️
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%> (ø)`
...xrpl/tx/invariants/PermissionedDomainInvariant.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/tx/invariants/VaultInvariant.cpp	`98.2% <100.0%> (ø)`
... and 33 more

... and 1 file 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.

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

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

                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

                        [](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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

github-actions · 2026-03-30T15:46:30Z

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

github-actions · 2026-03-30T16:39:22Z

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

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

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

Comment thread src/xrpld/rpc/detail/RPCCall.cpp

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

Comment thread src/libxrpl/shamap/SHAMap.cpp Outdated

Comment thread src/xrpld/app/consensus/RCLCxPeerPos.h

Update src/libxrpl/shamap/SHAMap.cpp

e081dec

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

godexsoft requested a review from kuznetsss March 30, 2026 14:16

kuznetsss approved these changes Mar 30, 2026

View reviewed changes

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

f451422

github-actions Bot added the PR: has conflicts label Mar 30, 2026

godexsoft added 2 commits March 30, 2026 16:48

Autofixes for performance checks

012c51b

Fix remaining issues from checks

145e2b5

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

6001e41

github-actions Bot removed the PR: has conflicts label Mar 30, 2026

godexsoft added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 30, 2026

bthomee added this pull request to the merge queue Mar 30, 2026

Merged via the queue into XRPLF:develop with commit ab8c168 Mar 30, 2026
3 checks passed

bthomee pushed a commit that referenced this pull request Mar 30, 2026

chore: Enable remaining clang-tidy performance checks (#6648)

ade241e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

bthomee pushed a commit that referenced this pull request Mar 30, 2026

chore: Enable remaining clang-tidy performance checks (#6648)

453d94d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

mvadari added this to the 3.2.0 milestone May 20, 2026

beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026

chore: Enable remaining clang-tidy performance checks (XRPLF#6648)

1cdecdc

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Uh oh!

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!

godexsoft Mar 30, 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!

Uh oh!

Uh oh!

github-actions Bot commented Mar 30, 2026

Uh oh!

github-actions Bot commented Mar 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

godexsoft commented Mar 25, 2026 •

edited

Loading

codecov Bot commented Mar 25, 2026 •

edited

Loading