[clang-tidy] let UseAfterMoveFinder::find() return an optional<UseAfterMove>

[tchaikov]

before this change, we use an output parameter so
UseAfterMoveFinder::find() can return the found UseAfterMove, and addition to it, UseAfterMoveFinder::find() return a bool, so we can tell if a use-after-free is identified. this arrangement could be confusing when one needs to understand when the each member variable of the returned UseAfterMove instance is initialized.

in this change, we return an std::optional<UseAfterMove> instead of a bool, so that it's more obvious on when/where the returned UseAfterMove is initialized.

this change is a cleanup. so it does not change the behavior of this check.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

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

Author: Kefu Chai (tchaikov)

Changes

before this change, we use an output parameter so
UseAfterMoveFinder::find() can return the found UseAfterMove, and addition to it, UseAfterMoveFinder::find() return a bool, so we can tell if a use-after-free is identified. this arrangement could be confusing when one needs to understand when the each member variable of the returned UseAfterMove instance is initialized.

in this change, we return an std::optional<UseAfterMove> instead of a bool, so that it's more obvious on when/where the returned UseAfterMove is initialized.

this change is a cleanup. so it does not change the behavior of this check.

---

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

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+26-27)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index c90c92b5f660a..a740b602af12b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -54,13 +54,12 @@ class UseAfterMoveFinder {
   // occurs after 'MovingCall' (the expression that performs the move). If a
   // use-after-move is found, writes information about it to 'TheUseAfterMove'.
   // Returns whether a use-after-move was found.
-  bool find(Stmt *CodeBlock, const Expr *MovingCall,
-            const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
+  std::optional<UseAfterMove> find(Stmt *CodeBlock, const Expr *MovingCall,
+                                   const DeclRefExpr *MovedVariable);
 
 private:
-  bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
-                    const ValueDecl *MovedVariable,
-                    UseAfterMove *TheUseAfterMove);
+  std::optional<UseAfterMove> findInternal(const CFGBlock *Block, const Expr *MovingCall,
+                                           const ValueDecl *MovedVariable);
   void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
                          llvm::SmallVectorImpl<const DeclRefExpr *> *Uses,
                          llvm::SmallPtrSetImpl<const Stmt *> *Reinits);
@@ -95,9 +94,8 @@ static StatementMatcher inDecltypeOrTemplateArg() {
 UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
     : Context(TheContext) {}
 
-bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
-                              const DeclRefExpr *MovedVariable,
-                              UseAfterMove *TheUseAfterMove) {
+std::optional<UseAfterMove> UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
+                                                     const DeclRefExpr *MovedVariable) {
   // Generate the CFG manually instead of through an AnalysisDeclContext because
   // it seems the latter can't be used to generate a CFG for the body of a
   // lambda.
@@ -111,7 +109,7 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
   std::unique_ptr<CFG> TheCFG =
       CFG::buildCFG(nullptr, CodeBlock, Context, Options);
   if (!TheCFG)
-    return false;
+    return std::nullopt;
 
   Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context);
   BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
@@ -125,10 +123,9 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
     MoveBlock = &TheCFG->getEntry();
   }
 
-  bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
-                            TheUseAfterMove);
+  auto TheUseAfterMove = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl());
 
-  if (Found) {
+  if (TheUseAfterMove) {
     if (const CFGBlock *UseBlock =
             BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef)) {
       // Does the use happen in a later loop iteration than the move?
@@ -142,15 +139,14 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
                                 : CFA.isReachable(UseBlock, MoveBlock);
     }
   }
-  return Found;
+  return TheUseAfterMove;
 }
 
-bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
-                                      const Expr *MovingCall,
-                                      const ValueDecl *MovedVariable,
-                                      UseAfterMove *TheUseAfterMove) {
+std::optional<UseAfterMove> UseAfterMoveFinder::findInternal(const CFGBlock *Block,
+                                                             const Expr *MovingCall,
+                                                             const ValueDecl *MovedVariable) {
   if (Visited.count(Block))
-    return false;
+    return std::nullopt;
 
   // Mark the block as visited (except if this is the block containing the
   // std::move() and it's being visited the first time).
@@ -189,17 +185,18 @@ bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
       }
 
       if (!HaveSavingReinit) {
-        TheUseAfterMove->DeclRef = Use;
+        UseAfterMove TheUseAfterMove;
+        TheUseAfterMove.DeclRef = Use;
 
         // Is this a use-after-move that depends on order of evaluation?
         // This is the case if the move potentially comes after the use (and we
         // already know that use potentially comes after the move, which taken
         // together tells us that the ordering is unclear).
-        TheUseAfterMove->EvaluationOrderUndefined =
+        TheUseAfterMove.EvaluationOrderUndefined =
             MovingCall != nullptr &&
             Sequence->potentiallyAfter(MovingCall, Use);
 
-        return true;
+        return TheUseAfterMove;
       }
     }
   }
@@ -208,12 +205,15 @@ bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
   // successors.
   if (Reinits.empty()) {
     for (const auto &Succ : Block->succs()) {
-      if (Succ && findInternal(Succ, nullptr, MovedVariable, TheUseAfterMove))
-        return true;
+      if (Succ) {
+        if (auto Found = findInternal(Succ, nullptr, MovedVariable)) {
+          return Found;
+        }
+      }
     }
   }
 
-  return false;
+  return std::nullopt;
 }
 
 void UseAfterMoveFinder::getUsesAndReinits(
@@ -518,9 +518,8 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
 
   for (Stmt *CodeBlock : CodeBlocks) {
     UseAfterMoveFinder Finder(Result.Context);
-    UseAfterMove Use;
-    if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
-      emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
+    if (auto Use = Finder.find(CodeBlock, MovingCall, Arg))
+      emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context,
                      determineMoveType(MoveDecl));
   }
 }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EugeneZelenko]

Please mention changes in Release Notes.

[PiotrZSL]

Please mention changes in Release Notes.

No need for, internal stuff.

[PiotrZSL]

LGTM