[clang-tools-extra] fc86350 - [clang-tidy] Improve performance-use-std-move in presence of control-flow (#184136)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 9 07:53:55 PDT 2026
Author: serge-sans-paille
Date: 2026-03-09T14:53:50Z
New Revision: fc863509e3afe19a3b24f70b31880708dc759b94
URL: https://github.com/llvm/llvm-project/commit/fc863509e3afe19a3b24f70b31880708dc759b94
DIFF: https://github.com/llvm/llvm-project/commit/fc863509e3afe19a3b24f70b31880708dc759b94.diff
LOG: [clang-tidy] Improve performance-use-std-move in presence of control-flow (#184136)
Added:
Modified:
clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
index ecd6a33aa722d..2a7df4142a6de 100644
--- a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
@@ -15,6 +15,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/STLExtras.h"
using namespace clang::ast_matchers;
@@ -22,8 +23,13 @@ using namespace clang::ast_matchers;
namespace clang::tidy::performance {
namespace {
-AST_MATCHER(CXXRecordDecl, hasNonTrivialMoveAssignment) {
- return Node.hasNonTrivialMoveAssignment();
+AST_MATCHER(CXXRecordDecl, hasAccessibleNonTrivialMoveAssignment) {
+ if (!Node.hasNonTrivialMoveAssignment())
+ return false;
+ for (const auto *CM : Node.methods())
+ if (CM->isMoveAssignmentOperator())
+ return !CM->isDeleted() && CM->getAccess() == AS_public;
+ llvm_unreachable("Move Assignment Operator Not Found");
}
AST_MATCHER(QualType, isLValueReferenceType) {
@@ -53,7 +59,8 @@ void UseStdMoveCheck::registerMatchers(MatchFinder *Finder) {
auto AssignOperatorExpr =
cxxOperatorCallExpr(
isCopyAssignmentOperator(),
- hasArgument(0, hasType(cxxRecordDecl(hasNonTrivialMoveAssignment()))),
+ hasArgument(0, hasType(cxxRecordDecl(
+ hasAccessibleNonTrivialMoveAssignment()))),
hasArgument(
1, declRefExpr(
to(varDecl(
@@ -93,14 +100,42 @@ void UseStdMoveCheck::check(const MatchFinder::MatchResult &Result) {
if (!TheCFG)
return;
- // Walk the CFG bottom-up, starting with the exit node.
- // TODO: traverse the whole CFG instead of only considering terminator
- // nodes.
+ // The algorithm to look for a convertible move-assign operator is the
+ // following: each node starts in the `Ready` state, with a number of
+ // `RemainingSuccessors` equal to its number of successors.
+ //
+ // Starting from the exit node, we walk the CFG backward. Whenever
+ // we meet a new block, we check if it either:
+ // 1. touches the `AssignValue`, in which case we stop the search, and mark
+ // each predecessor as not `Ready`. No predecessor walk.
+ // 2. contains a convertible copy-assign operator, in which case we generate a
+ // fix, and mark each predecessor as not Ready. No predecessor walk.
+ // 3. does not interact with `AssignValue`, in which case we decrement the
+ // `RemainingSuccessors` of each predecessor. And if it happens to turn to
+ // 0 while still being `Ready`, we add it to the `WorkList`.
+
+ struct BlockState {
+ bool Ready;
+ unsigned RemainingSuccessors;
+ };
+ llvm::DenseMap<const CFGBlock *, BlockState> CFGState;
+ for (const auto *B : *TheCFG)
+ CFGState.try_emplace(B, BlockState{true, B->succ_size()});
+
const CFGBlock &TheExit = TheCFG->getExit();
- for (auto &Pred : TheExit.preds()) {
- if (!Pred.isReachable())
+ std::vector<const CFGBlock *> WorkList = {&TheExit};
+
+ while (!WorkList.empty()) {
+ const CFGBlock *B = WorkList.back();
+ WorkList.pop_back();
+ const BlockState &BS = CFGState.find(B)->second;
+ if (!BS.Ready)
continue;
- for (const CFGElement &Elt : llvm::reverse(*Pred)) {
+
+ assert(BS.RemainingSuccessors == 0 &&
+ "All successors have been processed.");
+ bool ReferencesAssignedValue = false;
+ for (const CFGElement &Elt : llvm::reverse(*B)) {
if (Elt.getKind() != CFGElement::Kind::Statement)
continue;
@@ -112,13 +147,36 @@ void UseStdMoveCheck::check(const MatchFinder::MatchResult &Result) {
<< FixItHint::CreateReplacement(
AssignValue->getLocation(),
("std::move(" + AssignValueName + ")").str());
+ ReferencesAssignedValue = true;
break;
}
- // The reference is being referenced after the assignment, bail out.
+
+ // The reference is being referenced after the assignment.
if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt,
*Result.Context)
- .empty())
+ .empty()) {
+ ReferencesAssignedValue = true;
break;
+ }
+ }
+ if (ReferencesAssignedValue) {
+ // Cancel all predecessors.
+ for (const auto &S : B->preds()) {
+ if (!S.isReachable())
+ continue;
+ CFGState.find(&*S)->second.Ready = false;
+ }
+ } else {
+ // Or process the ready ones.
+ for (const auto &S : B->preds()) {
+ if (!S.isReachable())
+ continue;
+ auto &W = CFGState.find(&*S)->second;
+ if (W.Ready) {
+ if (--W.RemainingSuccessors == 0)
+ WorkList.push_back(&*S);
+ }
+ }
}
}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp
index 4492d976c37bd..1b011f2fb4f3c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp
@@ -33,6 +33,18 @@ struct NoMoveAssign {
NoMoveAssign& operator=(NoMoveAssign&&) = delete;
};
+struct NoDefaultedMoveAssign {
+ NoDefaultedMoveAssign& operator=(const NoDefaultedMoveAssign&);
+ NoDefaultedMoveAssign& operator=(NoDefaultedMoveAssign&&) = default;
+ NoMoveAssign Field;
+};
+
+struct PrivateMoveAssign {
+ PrivateMoveAssign& operator=(const PrivateMoveAssign&);
+ private:
+ PrivateMoveAssign& operator=(PrivateMoveAssign&&);
+};
+
template<class T>
void use(T&) {}
@@ -300,3 +312,78 @@ void InvalidMoveAssign(NoMoveAssign& target, NoMoveAssign source) {
// No message expected, moving is deleted.
target = source;
}
+
+void InvalidPrivateMoveAssign(PrivateMoveAssign& target, PrivateMoveAssign source) {
+ // No message expected, moving is private.
+ target = source;
+}
+
+void DeletedDefaultMoveAssign(NoDefaultedMoveAssign& target, NoDefaultedMoveAssign source) {
+ // No message expected, default move is invalid.
+ target = source;
+}
+
+// Check moving within control-flow
+// --------------------------------
+
+void ConvertibleNonTrivialMoveAssignWithinTestPred(bool pred, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+ target = source;
+ // CHECK-FIXES: target = std::move(source);
+ if (pred)
+ target = target;
+}
+
+void NonConvertibleNonTrivialMoveAssignWithinTestPred(bool pred, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ target = source;
+ if (pred)
+ source.stuff();
+}
+
+void ConvertibleNonTrivialMoveAssignWithinTestBothPred(bool pred, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+ target = source;
+ // CHECK-FIXES: target = std::move(source);
+ if (pred)
+ target = target;
+ else
+ target.stuff();
+}
+
+void NonConvertibleNonTrivialMoveAssignWithinLoop(unsigned count, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ while(--count)
+ target = source;
+}
+
+void ConvertibleNonTrivialMoveAssignWithinTestMultiplePred(bool pred0, bool pred1, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ if(pred0) {
+ // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+ target = source;
+ // CHECK-FIXES: target = std::move(source);
+ }
+ else {
+ if (pred1) {
+ // CHECK-MESSAGES: [[@LINE+1]]:16: warning: 'source' could be moved here [performance-use-std-move]
+ target = source;
+ // CHECK-FIXES: target = std::move(source);
+ }
+ else {
+ target.stuff();
+ }
+ }
+}
+
+void NonConvertibleNonTrivialMoveAssignWithinTestMultiplePred(bool pred0, bool pred1, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+ target = source;
+ if(pred0) {
+ target.stuff();
+ }
+ else {
+ // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+ target = source;
+ // CHECK-FIXES: target = std::move(source);
+ if (pred1) {
+ target.stuff();
+ }
+ }
+}
More information about the cfe-commits
mailing list