chore: Enable clang-tidy use-after-move check#6432
chore: Enable clang-tidy use-after-move check#6432kuznetsss wants to merge 3 commits intoXRPLF:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables the clang-tidy bugprone-use-after-move check and addresses all instances where use-after-move was detected. The changes are primarily test-related, where tests intentionally access moved-from objects to verify their post-move state. The production code change in SHAMap adds an explicit reset to prevent use-after-move, though this may be redundant.
Changes:
- Enabled
bugprone-use-after-movecheck in.clang-tidyconfiguration - Added NOLINT suppressions to test code where intentional use-after-move validates moved-from object state
- Added explicit
prevNodereset inSHAMap::delItem()to prevent use-after-move warning
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.clang-tidy |
Enabled bugprone-use-after-move check |
src/tests/libxrpl/json/Value.cpp |
Added NOLINT suppressions for intentional use-after-move in Json::Value move tests |
src/test/protocol/STObject_test.cpp |
Added NOLINT suppressions for Buffer move verification tests |
src/test/jtx/Env_test.cpp |
Added NOLINT suppressions for JTx move semantics tests |
src/test/core/ClosureCounter_test.cpp |
Added NOLINT suppression for verifying moved string is empty |
src/test/basics/Buffer_test.cpp |
Added NOLINT suppressions for comprehensive Buffer move operation tests |
src/libxrpl/shamap/SHAMap.cpp |
Added explicit prevNode reset after move to prevent use-after-move; includes TODO noting potential redundancy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6432 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 848 848
Lines 67757 67758 +1
Branches 7558 7558
=========================================
- Hits 54074 54065 -9
- Misses 13683 13693 +10
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| node = unshareNode(std::move(node), nodeID); | ||
| node->setChild(selectBranch(nodeID, id), std::move(prevNode)); | ||
| prevNode = intr_ptr::SharedPtr<SHAMapTreeNode>{}; |
There was a problem hiding this comment.
This reinitializes prevNode immediately after moving it into setChild. Consider using std::exchange(prevNode, {}) as the argument to setChild so the move-and-clear is a single, self-documenting operation and avoids the extra assignment line.
High Level Overview of Change
This PR enables use-after-move clang-tidy check. It also fixes all the places where use after move was found.
Context of Change
Use after move was found only in tests checking the state of object after it was moved.
The fix in SHAmap doesn't change much, because the pointer is already reset after move.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)