Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -58,11 +58,11 @@ public: UseAfterMoveFinder(ASTContext *TheContext); - // Within the given function body, finds the first use of 'MovedVariable' that + // Within the given code block, finds the first use of 'MovedVariable' that // 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 *FunctionBody, const Expr *MovingCall, + bool find(Stmt *CodeBlock, const Expr *MovingCall, const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove); private: @@ -104,7 +104,7 @@ UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext) : Context(TheContext) {} -bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall, +bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall, const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove) { // Generate the CFG manually instead of through an AnalysisDeclContext because @@ -118,12 +118,11 @@ Options.AddImplicitDtors = true; Options.AddTemporaryDtors = true; std::unique_ptr TheCFG = - CFG::buildCFG(nullptr, FunctionBody, Context, Options); + CFG::buildCFG(nullptr, CodeBlock, Context, Options); if (!TheCFG) return false; - Sequence = - std::make_unique(TheCFG.get(), FunctionBody, Context); + Sequence = std::make_unique(TheCFG.get(), CodeBlock, Context); BlockMap = std::make_unique(TheCFG.get(), Context); Visited.clear(); @@ -399,18 +398,28 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { auto CallMoveMatcher = - callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), - hasArgument(0, declRefExpr().bind("arg")), - anyOf(hasAncestor(compoundStmt( - hasParent(lambdaExpr().bind("containing-lambda")))), - hasAncestor(functionDecl().bind("containing-func"))), - unless(inDecltypeOrTemplateArg()), - // try_emplace is a common maybe-moving function that returns a - // bool to tell callers whether it moved. Ignore std::move inside - // try_emplace to avoid false positives as we don't track uses of - // the bool. - unless(hasParent(cxxMemberCallExpr( - callee(cxxMethodDecl(hasName("try_emplace"))))))) + callExpr( + callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), + hasArgument(0, declRefExpr().bind("arg")), + anyOf(hasAncestor( + stmt( + // For some reason cxxCtorInitializer didn't + // match, so work around by matching the topmost + // Stmt inside the CXXCtorInitializer. + hasParent(cxxConstructorDecl().bind("containing-ctor")), + // Exclude the constructor body + unless(compoundStmt())) + .bind("containing-ctor-init-stmt")), + hasAncestor(compoundStmt( + hasParent(lambdaExpr().bind("containing-lambda")))), + hasAncestor(functionDecl().bind("containing-func"))), + unless(inDecltypeOrTemplateArg()), + // try_emplace is a common maybe-moving function that returns a + // bool to tell callers whether it moved. Ignore std::move inside + // try_emplace to avoid false positives as we don't track uses of + // the bool. + unless(hasParent(cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("try_emplace"))))))) .bind("call-move"); Finder->addMatcher( @@ -434,6 +443,10 @@ } void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *ContainingCtor = + Result.Nodes.getNodeAs("containing-ctor"); + const auto *ContainingCtorInitStmt = + Result.Nodes.getNodeAs("containing-ctor-init-stmt"); const auto *ContainingLambda = Result.Nodes.getNodeAs("containing-lambda"); const auto *ContainingFunc = @@ -445,23 +458,47 @@ if (!MovingCall || !MovingCall->getExprLoc().isValid()) MovingCall = CallMove; - Stmt *FunctionBody = nullptr; - if (ContainingLambda) - FunctionBody = ContainingLambda->getBody(); - else if (ContainingFunc) - FunctionBody = ContainingFunc->getBody(); - else - return; - // Ignore the std::move if the variable that was passed to it isn't a local // variable. if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) return; - UseAfterMoveFinder Finder(Result.Context); - UseAfterMove Use; - if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use)) - emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + // Collect all code blocks that could use the arg after move. + llvm::SmallVector CodeBlocks{}; + if (ContainingCtor) { + CodeBlocks.push_back(ContainingCtor->getBody()); + if (ContainingCtorInitStmt) { + // Find the constructor initializer's member field. + FieldDecl *ContainingCtorField{nullptr}; + for (CXXCtorInitializer *Init : ContainingCtor->inits()) { + if (Init->getInit() == ContainingCtorInitStmt) { + ContainingCtorField = Init->getMember(); + break; + } + } + if (ContainingCtorField) { + // Collect the constructor initializer expressions. + for (CXXCtorInitializer *Init : ContainingCtor->inits()) { + if (Init->getMember() && Init->getMember()->getFieldIndex() >= + ContainingCtorField->getFieldIndex()) { + if (Init->getInit()) + CodeBlocks.push_back(Init->getInit()); + } + } + } + } + } else if (ContainingLambda) + CodeBlocks.push_back(ContainingLambda->getBody()); + else if (ContainingFunc) { + CodeBlocks.push_back(ContainingFunc->getBody()); + } + + for (Stmt *CodeBlock : CodeBlocks) { + UseAfterMoveFinder Finder(Result.Context); + UseAfterMove Use; + if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use)) + emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + } } } // namespace clang::tidy::bugprone Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -187,6 +187,10 @@ ` check. Global options of the same name should be used instead. +- Improved :doc:`bugprone-use-after-move + ` check to also cover constructor + initializers. + - In :doc:`modernize-use-default-member-init ` count template constructors toward hand written constructors so that they are skipped if more Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp @@ -1353,6 +1353,70 @@ } } // namespace UnevalContext +class CtorInit { +public: + CtorInit(std::string val) + : a{val.empty()}, // fine + s{std::move(val)}, + b{val.empty()} + // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved + // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here + {} + +private: + bool a; + std::string s; + bool b; +}; + +class CtorInitLambda { +public: + CtorInitLambda(std::string val) + : a{val.empty()}, // fine + s{std::move(val)}, + b{[&] { return val.empty(); }()}, + // CHECK-NOTES: [[@LINE-1]]:12: warning: 'val' used after it was moved + // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here + c{[] { + std::string str{}; + std::move(str); + return str.empty(); + // CHECK-NOTES: [[@LINE-1]]:18: warning: 'str' used after it was moved + // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here + }()} { + std::move(val); + // CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved + // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here + std::string val2{}; + std::move(val2); + val2.empty(); + // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val2' used after it was moved + // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here + } + +private: + bool a; + std::string s; + bool b; + bool c; +}; + +class CtorInitOrder { +public: + CtorInitOrder(std::string val) + : a{val.empty()}, // fine + b{val.empty()}, + // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved + s{std::move(val)} {} // wrong order + // CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move + +private: + bool a; + std::string s; + bool b; +}; + class PR38187 { public: PR38187(std::string val) : val_(std::move(val)) {