diff options
5 files changed, 123 insertions, 46 deletions
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp index edc3cf20999..4c123181fcb 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -47,14 +47,14 @@ bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function, return !Matches.empty(); } -bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt, +bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl, ASTContext &Context) { auto Matches = - match(findAll(declRefExpr( + match(decl(forEachDescendant(declRefExpr( equalsNode(&DeclRef), unless(hasAncestor(stmt(anyOf(forStmt(), cxxForRangeStmt(), - whileStmt(), doStmt())))))), - Stmt, Context); + whileStmt(), doStmt()))))))), + Decl, Context); return Matches.empty(); } @@ -72,7 +72,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { unless(referenceType())))), decl().bind("param")); Finder->addMatcher( - functionDecl(isDefinition(), + functionDecl(hasBody(stmt()), isDefinition(), unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))), unless(isInstantiated()), has(typeLoc(forEach(ExpensiveValueParamDecl))), @@ -89,22 +89,13 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { bool IsConstQualified = Param->getType().getCanonicalType().isConstQualified(); - // Skip declarations delayed by late template parsing without a body. - if (!Function->getBody()) - return; - - // Do not trigger on non-const value parameters when: - // 1. they are in a constructor definition since they can likely trigger - // modernize-pass-by-value which will suggest to move the argument. - if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) || - !Function->doesThisDeclarationHaveABody())) - return; - auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs( - *Param, *Function->getBody(), *Result.Context); + *Param, *Function, *Result.Context); auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs( - *Param, *Function->getBody(), *Result.Context); - // 2. they are not only used as const. + *Param, *Function, *Result.Context); + + // Do not trigger on non-const value parameters when they are not only used as + // const. if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs)) return; @@ -113,20 +104,18 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { // move assignment operator and is only referenced once when copy-assigned. // In this case wrap DeclRefExpr with std::move() to avoid the unnecessary // copy. - if (!IsConstQualified) { + if (!IsConstQualified && AllDeclRefExprs.size() == 1) { auto CanonicalType = Param->getType().getCanonicalType(); - if (AllDeclRefExprs.size() == 1 && - !hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(), - *Result.Context) && + const auto &DeclRefExpr = **AllDeclRefExprs.begin(); + + if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) && ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) && utils::decl_ref_expr::isCopyConstructorArgument( - **AllDeclRefExprs.begin(), *Function->getBody(), - *Result.Context)) || + DeclRefExpr, *Function, *Result.Context)) || (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) && utils::decl_ref_expr::isCopyAssignmentArgument( - **AllDeclRefExprs.begin(), *Function->getBody(), - *Result.Context)))) { - handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context); + DeclRefExpr, *Function, *Result.Context)))) { + handleMoveFix(*Param, DeclRefExpr, *Result.Context); return; } } diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp index 11d0c311f14..a28b1d25a04 100644 --- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp +++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp @@ -71,6 +71,40 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, return DeclRefs; } +// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl +// is the a const reference or value argument to a CallExpr or CXXConstructExpr. +SmallPtrSet<const DeclRefExpr *, 16> +constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, + ASTContext &Context) { + auto DeclRefToVar = + declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"); + auto ConstMethodCallee = callee(cxxMethodDecl(isConst())); + // Match method call expressions where the variable is referenced as the this + // implicit object argument and opertor call expression for member operators + // where the variable is the 0-th argument. + auto Matches = + match(decl(forEachDescendant(expr( + anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)), + cxxOperatorCallExpr(ConstMethodCallee, + hasArgument(0, DeclRefToVar)))))), + Decl, Context); + SmallPtrSet<const DeclRefExpr *, 16> DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + auto ConstReferenceOrValue = + qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))), + unless(anyOf(referenceType(), pointerType())))); + auto UsedAsConstRefOrValueArg = forEachArgumentWithParam( + DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue))); + Matches = match(decl(forEachDescendant(callExpr(UsedAsConstRefOrValueArg))), + Decl, Context); + extractNodesByIdTo(Matches, "declRef", DeclRefs); + Matches = + match(decl(forEachDescendant(cxxConstructExpr(UsedAsConstRefOrValueArg))), + Decl, Context); + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt, ASTContext &Context) { // Collect all DeclRefExprs to the loop variable and all CallExprs and @@ -93,30 +127,41 @@ allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) { return DeclRefs; } -bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, +SmallPtrSet<const DeclRefExpr *, 16> +allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context) { + auto Matches = match( + decl(forEachDescendant( + declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))), + Decl, Context); + SmallPtrSet<const DeclRefExpr *, 16> DeclRefs; + extractNodesByIdTo(Matches, "declRef", DeclRefs); + return DeclRefs; +} + +bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl, ASTContext &Context) { auto UsedAsConstRefArg = forEachArgumentWithParam( declRefExpr(equalsNode(&DeclRef)), parmVarDecl(hasType(matchers::isReferenceToConst()))); auto Matches = match( - stmt(hasDescendant( + decl(hasDescendant( cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl( isCopyConstructor()))) .bind("constructExpr"))), - Stmt, Context); + Decl, Context); return !Matches.empty(); } -bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, +bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl, ASTContext &Context) { auto UsedAsConstRefArg = forEachArgumentWithParam( declRefExpr(equalsNode(&DeclRef)), parmVarDecl(hasType(matchers::isReferenceToConst()))); auto Matches = match( - stmt(hasDescendant( + decl(hasDescendant( cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("=")) .bind("operatorCallExpr"))), - Stmt, Context); + Decl, Context); return !Matches.empty(); } diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h index 5d56f7f05d9..c25102f84a4 100644 --- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h +++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h @@ -28,23 +28,34 @@ namespace decl_ref_expr { bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt, ASTContext &Context); -// Returns set of all DeclRefExprs to VarDecl in Stmt. +/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``. llvm::SmallPtrSet<const DeclRefExpr *, 16> allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context); -// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in -// a const fashion. +/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl``. +llvm::SmallPtrSet<const DeclRefExpr *, 16> +allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context); + +/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where +/// ``VarDecl`` is guaranteed to be accessed in a const fashion. llvm::SmallPtrSet<const DeclRefExpr *, 16> constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context); -// Returns true if DeclRefExpr is the argument of a copy-constructor call expr. -bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, +/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl`` where +/// ``VarDecl`` is guaranteed to be accessed in a const fashion. +llvm::SmallPtrSet<const DeclRefExpr *, 16> +constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, + ASTContext &Context); + +/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor +/// call expression within ``Decl``. +bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl, ASTContext &Context); -// Returns true if DeclRefExpr is the argument of a copy-assignment operator -// call expr. -bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt, +/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-assignment +/// operator CallExpr within ``Decl``. +bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl, ASTContext &Context); } // namespace decl_ref_expr diff --git a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp index a712305dd15..623bf8b3082 100644 --- a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp +++ b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp @@ -64,6 +64,7 @@ void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToC struct PositiveConstValueConstructor { PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {} // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy' + // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {} }; template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { @@ -125,8 +126,17 @@ void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) { Obj.nonConstMethod(); } -struct NegativeValueCopyConstructor { - NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {} +struct PositiveValueUnusedConstructor { + PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {} + // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy' + // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {} +}; + +struct PositiveValueCopiedConstructor { + PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {} + // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy' + // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {} + ExpensiveToCopyType Field; }; template <typename T> diff --git a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param.cpp index 18fcf0ffca2..cf12d2ccd8c 100644 --- a/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param.cpp +++ b/clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param.cpp @@ -82,6 +82,7 @@ void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToC struct PositiveConstValueConstructor { PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {} // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy' + // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {} }; template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) { @@ -143,8 +144,29 @@ void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) { Obj.nonConstMethod(); } -struct NegativeValueCopyConstructor { - NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {} +struct PositiveValueUnusedConstructor { + PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {} + // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy' + // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {} +}; + +struct PositiveValueCopiedConstructor { + PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {} + // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy' + // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {} + ExpensiveToCopyType Field; +}; + +struct PositiveValueMovableConstructor { + PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(Copy) {} + // CHECK-MESSAGES: [[@LINE-1]]:70: warning: parameter 'Copy' + // CHECK-FIXES: PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(std::move(Copy)) {} + ExpensiveMovableType Field; +}; + +struct NegativeValueMovedConstructor { + NegativeValueMovedConstructor(ExpensiveMovableType Copy) : Field(static_cast<ExpensiveMovableType &&>(Copy)) {} + ExpensiveMovableType Field; }; template <typename T> |
