⚡ Quick Guide: chore: Enable clang-tidy `bugprone-use-after-move` check by godexsoft · Pull Request #6476 · XRPLF/rippled · GitHub

godexsoft · 2026-03-04T17:53:31Z

High Level Overview of Change

This PR enables clang-tidy bugprone-use-after-move check.

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

No impact.

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

codecov · 2026-03-04T18:35:07Z

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (9e14707) to head (76df614).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/libxrpl/shamap/SHAMap.cpp	83.3%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6476     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          878     878             
  Lines        68026   68028      +2     
  Branches      7556    7555      -1     
=========================================
- Hits         54309   54302      -7     
- Misses       13717   13726      +9

Files with missing lines	Coverage Δ
src/libxrpl/shamap/SHAMap.cpp	`91.4% <83.3%> (+<0.1%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

…r-move

godexsoft

Seems like an old .reset() might not be needed anymore. @ximinez if you could take a look and advise, would be great.

…r-move

Copilot

Pull request overview

Enables the clang-tidy bugprone-use-after-move check and updates existing code/tests to accommodate the new diagnostic by adding targeted suppressions where moved-from state is intentionally inspected.

Changes:

Enable bugprone-use-after-move in .clang-tidy.
Add // NOLINT(bugprone-use-after-move) suppressions in multiple unit tests that intentionally validate moved-from behavior.
Add a suppression (and TODO) in SHAMap::delItem around a moved-from pointer reset.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`.clang-tidy`	Turns on `bugprone-use-after-move` check.
`src/tests/libxrpl/json/Value.cpp`	Suppresses intentional moved-from inspections in JSON value tests.
`src/test/protocol/STObject_test.cpp`	Suppresses moved-from `Buffer` checks after move into `STObject`.
`src/test/jtx/Env_test.cpp`	Suppresses moved-from `JTx` state checks in move tests.
`src/test/core/ClosureCounter_test.cpp`	Suppresses moved-from string state check after forwarding/move.
`src/test/basics/Buffer_test.cpp`	Suppresses moved-from `Buffer` validation in move construction/assignment tests.
`src/libxrpl/shamap/SHAMap.cpp`	Suppresses a use-after-move warning on `prevNode.reset()` in `delItem`.

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

ximinez

I'm only flagging this as "request changes" to point out this comment: #6476 (comment)

Otherwise, this looks fine.

…r-move

ximinez

The updates look good, but I did have an alternative suggestion below. Whether it works or not, if you make the change or not, just re-request, and I'll approve.

ximinez · 2026-03-10T21:05:11Z

            {
-                // no children below this branch
-                prevNode.reset();
+                XRPL_ASSERT(
+                    not prevNode,  // NOLINT(bugprone-use-after-move)
+                    "xrpl::SHAMap::delItem : There should be no children below this branch");
            }


I'm looking fresh at the context around this change. It's in a loop and prevNode is reused on each iteration. I didn't notice that before. The old reset() is ensuring that prevNode is holding a nullptr for the next loop, juuuust in case the std::move doesn't do what we expect. But that upsets the linter. OTOH, the assignments below do not upset the linter. Have you tried doing a direct assignment like the ones below? prevNode = nullptr; or prevNode = intr_ptr::SharedPtr<SHAMapTreeNode>{}?

If that works, I think that would be a safer way to go. You can move or remove the assert.

Let's try! In less critical code i would say this is redundant but it's SHAMap so I'll do anything to stay safe here 👍

This line attracted my attention too. After reviewing reset() vs operator=(SharedIntrusive&&), the assignment is technically redundant—move semantics already guarantee prevNode.ptr_ becomes nullptr via unsafeExchange(nullptr).

That said, I'm fine keeping it for code clarity. The explicit assignment prevNode = TreeNodeType{} makes the intent obvious to readers who aren't familiar with SharedIntrusive internals, even though it performs no actual refcount operations (the null check in unsafeReleaseAndStore causes an early return).

Not a safety issue, just a style preference for defensive clarity.

[nit] Another alternative would be to use std::exchange here:

node->setChild( selectBranch(nodeID, id), std::exchange(prevNode, TreeNodeType{}));

This keeps the explicit reset while avoiding the moved-from state entirely, which allows
removing the // NOLINT(bugprone-use-after-move) suppression.

Agree with your assessment and I too think that this is a good alternative. But i'm somewhat hesitant to touch SHAMap code in general so prefer to leave the ASSERT and explicit assignment at this time 👍

@vlntb is this something you could pick in as a follow-up PR, given you're much more familiar with the SHAMap?

Sure, it requires both SHAMap and intrusive pointers knowledge - I'm happy to do a follow-up on this, even if it is not an essential change.

…r-move

vlntb

LGTM. Added an optional suggestion.

…r-move

Enable new check

7801aa3

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

godexsoft added 3 commits March 9, 2026 14:05

Fix found clang-tidy issue

fbcaa0c

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

b5483d9

…r-move

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

453296e

…r-move

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

godexsoft marked this pull request as ready for review March 9, 2026 15:12

godexsoft commented Mar 9, 2026

View reviewed changes

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

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

a9cd7ef

…r-move

bthomee requested review from Copilot and ximinez March 10, 2026 14:58

Copilot started reviewing on behalf of bthomee March 10, 2026 14:58 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

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

ximinez requested changes Mar 10, 2026

View reviewed changes

godexsoft added 4 commits March 10, 2026 17:23

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

0a8687a

…r-move

Assert instead of reset

c51d1d6

Try different approach to remove nolint

fe73d2c

Better

04a4164

godexsoft closed this Mar 10, 2026

godexsoft reopened this Mar 10, 2026

godexsoft mentioned this pull request Mar 10, 2026

chore: Enable clang-tidy use-after-move check #6432

Closed

13 tasks

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

10a5875

…r-move

godexsoft requested a review from ximinez March 10, 2026 17:46

Well, can't win them all

5270ca9

ximinez requested changes Mar 10, 2026

View reviewed changes

godexsoft added 4 commits March 11, 2026 12:08

Implement review suggestion

7a5a885

Add back original comment

af261a2

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

fd9dc52

…r-move

Fix compilation error

a01a8ee

Fix merge conflict

4cb3337

godexsoft requested a review from ximinez March 16, 2026 16:06

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

6beee16

…r-move

godexsoft requested a review from bthomee March 16, 2026 16:08

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

4804925

…r-move

bthomee requested review from vlntb and removed request for bthomee March 17, 2026 14:16

ximinez approved these changes Mar 17, 2026

View reviewed changes

vlntb approved these changes Mar 17, 2026

View reviewed changes

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 17, 2026

godexsoft and others added 2 commits March 17, 2026 22:57

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

ccd7717

…r-move

Merge branch 'develop' into chore/clang-tidy-checks-bugprone-use-afte…

76df614

…r-move

bthomee enabled auto-merge (squash) March 17, 2026 23:52

bthomee merged commit 2a325e7 into XRPLF:develop Mar 18, 2026
1 check passed

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 clang-tidy bugprone-use-after-move check (XRPLF#6476)

fc125d9

Uh oh!

Conversation

godexsoft commented Mar 4, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

godexsoft Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

vlntb Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

vlntb Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

godexsoft Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

bthomee Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

vlntb Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

vlntb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Mar 4, 2026 •

edited

Loading