[clang-tools-extra] [clang-tidy] Improve performance-use-std-move in presence of control-… (PR #184136)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: None (serge-sans-paille)

<details>
<summary>Changes</summary>

…flow

---
Full diff: https://github.com/llvm/llvm-project/pull/184136.diff


2 Files Affected:

- (modified) clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp (+39-8) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp (+65) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
index ecd6a33aa722d..a71badf341cfc 100644
--- a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
@@ -93,14 +93,27 @@ 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.
+  struct BlockState {
+    bool Ready;
+    unsigned RemainingSuccessors;
+  };
+  std::unordered_map<const CFGBlock *, BlockState> CFGState;
+  for (const auto *B : *TheCFG)
+    CFGState.emplace(B, BlockState{true, B->succ_size()});
+
   const CFGBlock &TheExit = TheCFG->getExit();
-  for (auto &Pred : TheExit.preds()) {
-    if (!Pred.isReachable())
+  std::vector<const CFGBlock *> ToVisit = {&TheExit};
+
+  // Walk the CFG bottom-up, starting with the exit node.
+  while (!ToVisit.empty()) {
+    const CFGBlock *B = ToVisit.back();
+    ToVisit.pop_back();
+    if (!CFGState.find(B)->second.Ready)
       continue;
-    for (const CFGElement &Elt : llvm::reverse(*Pred)) {
+
+    assert(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 +125,31 @@ 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())
+        CFGState.find(&*S)->second.Ready = false;
+    } else {
+      // Or process the ready ones.
+      for (const auto &S : B->preds()) {
+        auto &W = CFGState.find(&*S)->second;
+        if (W.Ready) {
+          if (--W.RemainingSuccessors == 0 && S.isReachable())
+            ToVisit.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..1a6c31feca3d4 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
@@ -300,3 +300,68 @@ void InvalidMoveAssign(NoMoveAssign& target, NoMoveAssign source) {
   // No message expected, moving is deleted.
   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();
+    }
+  }
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/184136