https://github.com/tchaikov created
https://github.com/llvm/llvm-project/pull/98100
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.
>From 3537fe828bac5678970e106ac89306e465446163 Mon Sep 17 00:00:00 2001
From: Kefu Chai <kefu.c...@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.c...@scylladb.com>
---
.../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 53 +++++++++----------
1 file changed, 26 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..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));
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
