Author: shuaiwang Date: Fri Aug 3 10:23:37 2018 New Revision: 338903 URL: http://llvm.org/viewvc/llvm-project?rev=338903&view=rev Log: Use ExprMutationAnalyzer in performance-unnecessary-value-param
Summary: This yields better recall as ExprMutationAnalyzer is more accurate. One common pattern this check is now able to catch is: ``` void foo(std::vector<X> v) { for (const auto& elm : v) { // ... } } ``` Reviewers: george.karpenkov Subscribers: a.sidorin, cfe-commits Differential Revision: https://reviews.llvm.org/D50102 Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp?rev=338903&r1=338902&r2=338903&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp Fri Aug 3 10:23:37 2018 @@ -10,6 +10,7 @@ #include "UnnecessaryValueParamCheck.h" #include "../utils/DeclRefExprUtils.h" +#include "../utils/ExprMutationAnalyzer.h" #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" #include "../utils/TypeTraits.h" @@ -31,14 +32,6 @@ std::string paramNameOrIndex(StringRef N .str(); } -template <typename S> -bool isSubset(const S &SubsetCandidate, const S &SupersetCandidate) { - for (const auto &E : SubsetCandidate) - if (SupersetCandidate.count(E) == 0) - return false; - return true; -} - bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function, ASTContext &Context) { auto Matches = match(declRefExpr(to(functionDecl(equalsNode(&Function))), @@ -98,43 +91,55 @@ void UnnecessaryValueParamCheck::registe void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) { const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param"); const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl"); - const size_t Index = std::find(Function->parameters().begin(), - Function->parameters().end(), Param) - - Function->parameters().begin(); - bool IsConstQualified = - Param->getType().getCanonicalType().isConstQualified(); - auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs( - *Param, *Function, *Result.Context); - auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs( - *Param, *Function, *Result.Context); - - // Do not trigger on non-const value parameters when they are not only used as - // const. - if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs)) + // Do not trigger on non-const value parameters when they are mutated either + // within the function body or within init expression(s) when the function is + // a ctor. + if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context) + .isMutated(Param)) return; + // CXXCtorInitializer might also mutate Param but they're not part of function + // body, so check them separately here. + if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) { + for (const auto *Init : Ctor->inits()) { + if (utils::ExprMutationAnalyzer(Init->getInit(), Result.Context) + .isMutated(Param)) + return; + } + } + + const bool IsConstQualified = + Param->getType().getCanonicalType().isConstQualified(); // If the parameter is non-const, check if it has a move constructor and is // only referenced once to copy-construct another object or whether it has a // 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 && AllDeclRefExprs.size() == 1) { - auto CanonicalType = Param->getType().getCanonicalType(); - const auto &DeclRefExpr = **AllDeclRefExprs.begin(); - - if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) && - ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) && - utils::decl_ref_expr::isCopyConstructorArgument( - DeclRefExpr, *Function, *Result.Context)) || - (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) && - utils::decl_ref_expr::isCopyAssignmentArgument( - DeclRefExpr, *Function, *Result.Context)))) { - handleMoveFix(*Param, DeclRefExpr, *Result.Context); - return; + if (!IsConstQualified) { + auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs( + *Param, *Function, *Result.Context); + if (AllDeclRefExprs.size() == 1) { + auto CanonicalType = Param->getType().getCanonicalType(); + const auto &DeclRefExpr = **AllDeclRefExprs.begin(); + + if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) && + ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) && + utils::decl_ref_expr::isCopyConstructorArgument( + DeclRefExpr, *Function, *Result.Context)) || + (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) && + utils::decl_ref_expr::isCopyAssignmentArgument( + DeclRefExpr, *Function, *Result.Context)))) { + handleMoveFix(*Param, DeclRefExpr, *Result.Context); + return; + } } } + const size_t Index = std::find(Function->parameters().begin(), + Function->parameters().end(), Param) - + Function->parameters().begin(); + auto Diag = diag(Param->getLocation(), IsConstQualified ? "the const qualified parameter %0 is " Modified: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp?rev=338903&r1=338902&r2=338903&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp Fri Aug 3 10:23:37 2018 @@ -15,6 +15,20 @@ void mutate(ExpensiveToCopyType *); void useAsConstReference(const ExpensiveToCopyType &); void useByValue(ExpensiveToCopyType); +template <class T> class Vector { + public: + using iterator = T*; + using const_iterator = const T*; + + Vector(const Vector&); + Vector& operator=(const Vector&); + + iterator begin(); + iterator end(); + const_iterator begin() const; + const_iterator end() const; +}; + // This class simulates std::pair<>. It is trivially copy constructible // and trivially destructible, but not trivially copy assignable. class SomewhatTrivial { @@ -59,6 +73,14 @@ void positiveExpensiveValue(ExpensiveToC useByValue(Obj); } +void positiveVector(Vector<ExpensiveToCopyType> V) { + // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'V' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveVector(const Vector<ExpensiveToCopyType>& V) { + for (const auto& Obj : V) { + useByValue(Obj); + } +} + void positiveWithComment(const ExpensiveToCopyType /* important */ S); // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S); void positiveWithComment(const ExpensiveToCopyType /* important */ S) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits