llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: mitchell (zeyi2) <details> <summary>Changes</summary> This change is a necessary step for a subsequent PR that will enhance the `bugprone-use-after-move` check to correctly handle cases where variables are re-initialized inside captured lambdas, which currently lead to FPs. Part of #<!-- -->172018 --- Full diff: https://github.com/llvm/llvm-project/pull/172219.diff 1 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp (+72-62) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b2e08fe688a1b..295453b093ee6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -31,6 +31,76 @@ using matchers::hasUnevaluatedContext; namespace { +static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { + return anyOf(hasAnyName("::std::move", "::std::forward"), + matchers::matchesAnyListedName(InvalidationFunctions)); +} + +StatementMatcher +makeReinitMatcher(const ValueDecl *MovedVariable, + llvm::ArrayRef<StringRef> InvalidationFunctions) { + const auto DeclRefMatcher = + declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); + + static const auto StandardContainerTypeMatcher = + hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(cxxRecordDecl(hasAnyName( + "::std::basic_string", "::std::vector", "::std::deque", + "::std::forward_list", "::std::list", "::std::set", "::std::map", + "::std::multiset", "::std::multimap", "::std::unordered_set", + "::std::unordered_map", "::std::unordered_multiset", + "::std::unordered_multimap")))))); + + static const auto StandardResettableOwnerTypeMatcher = hasType( + hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( + hasAnyName("::std::unique_ptr", "::std::shared_ptr", + "::std::weak_ptr", "::std::optional", "::std::any")))))); + + // Matches different types of reinitialization. + return stmt( + anyOf( + // Assignment. In addition to the overloaded assignment + // operator, test for built-in assignment as well, since + // template functions may be instantiated to use std::move() on + // built-in types. + binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)), + // Declaration. We treat this as a type of reinitialization + // too, so we don't need to treat it separately. + declStmt(hasDescendant(equalsNode(MovedVariable))), + // clear() and assign() on standard containers. + cxxMemberCallExpr( + on(expr(DeclRefMatcher, StandardContainerTypeMatcher)), + // To keep the matcher simple, we check for assign() calls + // on all standard containers, even though only vector, + // deque, forward_list and list have assign(). If assign() + // is called on any of the other containers, this will be + // flagged by a compile error anyway. + callee(cxxMethodDecl(hasAnyName("clear", "assign")))), + // reset() on standard smart pointers. + cxxMemberCallExpr(on(expr(DeclRefMatcher, + StandardResettableOwnerTypeMatcher)), + callee(cxxMethodDecl(hasName("reset")))), + // Methods that have the [[clang::reinitializes]] attribute. + cxxMemberCallExpr(on(DeclRefMatcher), + callee(cxxMethodDecl( + hasAttr(clang::attr::Reinitializes)))), + // Passing variable to a function as a non-const pointer. + callExpr(forEachArgumentWithParam( + unaryOperator(hasOperatorName("&"), + hasUnaryOperand(DeclRefMatcher)), + unless( + parmVarDecl(hasType(pointsTo(isConstQualified())))))), + // Passing variable to a function as a non-const lvalue + // reference (unless that function is std::move()). + callExpr(forEachArgumentWithParam( + traverse(TK_AsIs, DeclRefMatcher), + unless(parmVarDecl(hasType( + references(qualType(isConstQualified())))))), + unless(callee(functionDecl( + getNameMatcher(InvalidationFunctions))))))) + .bind("reinit"); +} + /// Contains information about a use-after-move. struct UseAfterMove { // The DeclRefExpr that constituted the use of the object. @@ -81,11 +151,6 @@ class UseAfterMoveFinder { } // namespace -static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { - return anyOf(hasAnyName("::std::move", "::std::forward"), - matchers::matchesAnyListedName(InvalidationFunctions)); -} - // Matches nodes that are // - Part of a decltype argument or class template argument (we check this by // seeing if they are children of a TypeLoc), or @@ -313,63 +378,8 @@ void UseAfterMoveFinder::getReinits( const CFGBlock *Block, const ValueDecl *MovedVariable, llvm::SmallPtrSetImpl<const Stmt *> *Stmts, llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) { - auto DeclRefMatcher = - declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); - - auto StandardContainerTypeMatcher = hasType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(cxxRecordDecl(hasAnyName( - "::std::basic_string", "::std::vector", "::std::deque", - "::std::forward_list", "::std::list", "::std::set", "::std::map", - "::std::multiset", "::std::multimap", "::std::unordered_set", - "::std::unordered_map", "::std::unordered_multiset", - "::std::unordered_multimap")))))); - - auto StandardResettableOwnerTypeMatcher = hasType( - hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( - hasAnyName("::std::unique_ptr", "::std::shared_ptr", - "::std::weak_ptr", "::std::optional", "::std::any")))))); - - // Matches different types of reinitialization. - auto ReinitMatcher = - stmt(anyOf( - // Assignment. In addition to the overloaded assignment operator, - // test for built-in assignment as well, since template functions - // may be instantiated to use std::move() on built-in types. - binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)), - // Declaration. We treat this as a type of reinitialization too, - // so we don't need to treat it separately. - declStmt(hasDescendant(equalsNode(MovedVariable))), - // clear() and assign() on standard containers. - cxxMemberCallExpr( - on(expr(DeclRefMatcher, StandardContainerTypeMatcher)), - // To keep the matcher simple, we check for assign() calls - // on all standard containers, even though only vector, - // deque, forward_list and list have assign(). If assign() - // is called on any of the other containers, this will be - // flagged by a compile error anyway. - callee(cxxMethodDecl(hasAnyName("clear", "assign")))), - // reset() on standard smart pointers. - cxxMemberCallExpr( - on(expr(DeclRefMatcher, StandardResettableOwnerTypeMatcher)), - callee(cxxMethodDecl(hasName("reset")))), - // Methods that have the [[clang::reinitializes]] attribute. - cxxMemberCallExpr( - on(DeclRefMatcher), - callee(cxxMethodDecl(hasAttr(clang::attr::Reinitializes)))), - // Passing variable to a function as a non-const pointer. - callExpr(forEachArgumentWithParam( - unaryOperator(hasOperatorName("&"), - hasUnaryOperand(DeclRefMatcher)), - unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))), - // Passing variable to a function as a non-const lvalue reference - // (unless that function is std::move()). - callExpr(forEachArgumentWithParam( - traverse(TK_AsIs, DeclRefMatcher), - unless(parmVarDecl(hasType( - references(qualType(isConstQualified())))))), - unless(callee(functionDecl( - getNameMatcher(InvalidationFunctions))))))) - .bind("reinit"); + const auto ReinitMatcher = + makeReinitMatcher(MovedVariable, InvalidationFunctions); Stmts->clear(); DeclRefs->clear(); `````````` </details> https://github.com/llvm/llvm-project/pull/172219 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
