[clang-tools-extra] [clang-tidy] let UseAfterMoveFinder::find() return an optional<UseAfterMove> (PR #98100)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 8 17:08:47 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-tools-extra
Author: Kefu Chai (tchaikov)
<details>
<summary>Changes</summary>
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
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));
}
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/98100
More information about the cfe-commits
mailing list