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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
