[clang-tools-extra] [clang-tidy] let UseAfterMoveFinder::find() return an optional<UseAfterMove> (PR #98100)

[Kefu Chai via cfe-commits]

https://github.com/tchaikov updated https://github.com/llvm/llvm-project/pull/98100

>From 1a2d88917db32253514fc7a533f5cb39d465a9c7 Mon Sep 17 00:00:00 2001
From: Kefu Chai <kefu.chai at scylladb.com>
Date: Tue, 9 Jul 2024 07:58:23 +0800
Subject: [PATCH] [clang-tidy] let UseAfterMoveFinder::find() return an
 optional<UseAfterMove>

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.

Signed-off-by: Kefu Chai <kefu.chai at scylladb.com>
---
 .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 56 ++++++++++---------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index c90c92b5f660a..8f4b5e8092dda 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -54,13 +54,13 @@ 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 +95,9 @@ 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 +111,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 +125,10 @@ 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 +142,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 +188,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 +208,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 +521,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));
   }
 }