This revision was automatically updated to reflect the committed changes. Closed by commit rL264146: Add check for unneeded copies of locals (authored by hokein).
Changed prior to commit: http://reviews.llvm.org/D18149?vs=51289&id=51391#toc Repository: rL LLVM http://reviews.llvm.org/D18149 Files: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -16,16 +16,32 @@ namespace clang { namespace tidy { namespace performance { +namespace { + +void recordFixes(const VarDecl &Var, ASTContext &Context, + DiagnosticBuilder &Diagnostic) { + // Do not propose fixes in macros since we cannot place them correctly. + if (Var.getLocation().isMacroID()) + return; + + Diagnostic << utils::create_fix_it::changeVarDeclToReference(Var, Context); + if (!Var.getType().isLocalConstQualified()) + Diagnostic << utils::create_fix_it::changeVarDeclToConst(Var); +} + +} // namespace + using namespace ::clang::ast_matchers; +using decl_ref_expr_utils::isOnlyUsedAsConst; -void UnnecessaryCopyInitialization::registerMatchers( - ast_matchers::MatchFinder *Finder) { +void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) { auto ConstReference = referenceType(pointee(qualType(isConstQualified()))); auto ConstOrConstReference = allOf(anyOf(ConstReference, isConstQualified()), unless(allOf(pointerType(), unless(pointerType(pointee( qualType(isConstQualified()))))))); + // Match method call expressions where the this argument is a const // type or const reference. This returned const reference is highly likely to // outlive the local const reference of the variable being declared. @@ -37,45 +53,82 @@ auto ConstRefReturningFunctionCall = callExpr(callee(functionDecl(returns(ConstReference))), unless(callee(cxxMethodDecl()))); + + auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) { + return compoundStmt( + forEachDescendant( + varDecl(hasLocalStorage(), + hasType(matchers::isExpensiveToCopy()), + hasInitializer(cxxConstructExpr( + hasDeclaration(cxxConstructorDecl( + isCopyConstructor())), + hasArgument(0, CopyCtorArg)) + .bind("ctorCall"))) + .bind("newVarDecl"))) + .bind("blockStmt"); + }; + Finder->addMatcher( - compoundStmt( - forEachDescendant( - varDecl( - hasLocalStorage(), hasType(matchers::isExpensiveToCopy()), - hasInitializer(cxxConstructExpr( - hasDeclaration(cxxConstructorDecl(isCopyConstructor())), - hasArgument( - 0, anyOf(ConstRefReturningFunctionCall, - ConstRefReturningMethodCallOfConstParam))))) - .bind("varDecl"))).bind("blockStmt"), + localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall, + ConstRefReturningMethodCallOfConstParam)), this); + + Finder->addMatcher(localVarCopiedFrom(declRefExpr( + to(varDecl(hasLocalStorage()).bind("oldVarDecl")))), + this); } void UnnecessaryCopyInitialization::check( - const ast_matchers::MatchFinder::MatchResult &Result) { - const auto *Var = Result.Nodes.getNodeAs<VarDecl>("varDecl"); + const MatchFinder::MatchResult &Result) { + const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl"); + const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>("oldVarDecl"); const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt"); - bool IsConstQualified = Var->getType().isConstQualified(); - if (!IsConstQualified && - !decl_ref_expr_utils::isOnlyUsedAsConst(*Var, *BlockStmt, - *Result.Context)) + const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall"); + + // A constructor that looks like T(const T& t, bool arg = false) counts as a + // copy only when it is called with default arguments for the arguments after + // the first. + for (unsigned int i = 1; i < CtorCall->getNumArgs(); ++i) + if (!CtorCall->getArg(i)->isDefaultArgument()) + return; + + if (OldVar == nullptr) { + handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Result.Context); + } else { + handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Result.Context); + } +} + +void UnnecessaryCopyInitialization::handleCopyFromMethodReturn( + const VarDecl &Var, const Stmt &BlockStmt, ASTContext &Context) { + bool IsConstQualified = Var.getType().isConstQualified(); + if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)) return; - DiagnosticBuilder Diagnostic = - diag(Var->getLocation(), - IsConstQualified ? "the const qualified variable '%0' is " + + auto Diagnostic = + diag(Var.getLocation(), + IsConstQualified ? "the const qualified variable %0 is " "copy-constructed from a const reference; " "consider making it a const reference" - : "the variable '%0' is copy-constructed from a " + : "the variable %0 is copy-constructed from a " "const reference but is only used as const " "reference; consider making it a const reference") - << Var->getName(); - // Do not propose fixes in macros since we cannot place them correctly. - if (Var->getLocStart().isMacroID()) + << &Var; + recordFixes(Var, Context, Diagnostic); +} + +void UnnecessaryCopyInitialization::handleCopyFromLocalVar( + const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, + ASTContext &Context) { + if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || + !isOnlyUsedAsConst(OldVar, BlockStmt, Context)) return; - Diagnostic << utils::create_fix_it::changeVarDeclToReference(*Var, - *Result.Context); - if (!IsConstQualified) - Diagnostic << utils::create_fix_it::changeVarDeclToConst(*Var); + + auto Diagnostic = diag(NewVar.getLocation(), + "local copy %0 of the variable %1 is never modified; " + "consider avoiding the copy") + << &NewVar << &OldVar; + recordFixes(NewVar, Context, Diagnostic); } } // namespace performance Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -30,6 +30,12 @@ : ClangTidyCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt, + ASTContext &Context); + void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar, + const Stmt &BlockStmt, ASTContext &Context); }; } // namespace performance Index: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst @@ -17,18 +17,21 @@ .. code-block:: c++ - const string& constReference() { - } - void function() { + const string& constReference(); + void Function() { // The warning will suggest making this a const reference. const string UnnecessaryCopy = constReference(); } struct Foo { const string& name() const; - } + }; void Function(const Foo& foo) { // The warning will suggest making this a const reference. - string UnnecessaryCopy = foo.name(); - UnnecessaryCopy.find("bar"); + string UnnecessaryCopy1 = foo.name(); + UnnecessaryCopy1.find("bar"); + + // The warning will suggest making this a const reference. + string UnnecessaryCopy2 = UnnecessaryCopy1; + UnnecessaryCopy2.find("bar"); } Index: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp +++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp @@ -1,28 +1,33 @@ // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t struct ExpensiveToCopyType { - ExpensiveToCopyType() {} - virtual ~ExpensiveToCopyType() {} - const ExpensiveToCopyType &reference() const { return *this; } - void nonConstMethod() {} + ExpensiveToCopyType(); + virtual ~ExpensiveToCopyType(); + const ExpensiveToCopyType &reference() const; + void nonConstMethod(); + bool constMethod() const; }; struct TrivialToCopyType { - const TrivialToCopyType &reference() const { return *this; } + const TrivialToCopyType &reference() const; }; -const ExpensiveToCopyType &ExpensiveTypeReference() { - static const ExpensiveToCopyType *Type = new ExpensiveToCopyType(); - return *Type; -} +struct WeirdCopyCtorType { + WeirdCopyCtorType(); + WeirdCopyCtorType(const WeirdCopyCtorType &w, bool oh_yes = true); -const TrivialToCopyType &TrivialTypeReference() { - static const TrivialToCopyType *Type = new TrivialToCopyType(); - return *Type; -} + void nonConstMethod(); + bool constMethod() const; +}; + +ExpensiveToCopyType global_expensive_to_copy_type; + +const ExpensiveToCopyType &ExpensiveTypeReference(); +const TrivialToCopyType &TrivialTypeReference(); void mutate(ExpensiveToCopyType &); void mutate(ExpensiveToCopyType *); +void useAsConstPointer(const ExpensiveToCopyType *); void useAsConstReference(const ExpensiveToCopyType &); void useByValue(ExpensiveToCopyType); @@ -203,23 +208,133 @@ ExpensiveToCopyType Obj; }; -#define UNNECESSARY_COPY_INIT_IN_MACRO_BODY(TYPE) \ - void functionWith##TYPE(const TYPE& T) { \ - auto AssignedInMacro = T.reference(); \ - } \ +#define UNNECESSARY_COPY_INIT_IN_MACRO_BODY(TYPE) \ + void functionWith##TYPE(const TYPE &T) { \ + auto AssignedInMacro = T.reference(); \ + } \ // Ensure fix is not applied. // CHECK-FIXES: auto AssignedInMacro = T.reference(); - UNNECESSARY_COPY_INIT_IN_MACRO_BODY(ExpensiveToCopyType) // CHECK-MESSAGES: [[@LINE-1]]:1: warning: the variable 'AssignedInMacro' is copy-constructed -#define UNNECESSARY_COPY_INIT_IN_MACRO_ARGUMENT(ARGUMENT) \ - ARGUMENT +#define UNNECESSARY_COPY_INIT_IN_MACRO_ARGUMENT(ARGUMENT) ARGUMENT void PositiveMacroArgument(const ExpensiveToCopyType &Obj) { UNNECESSARY_COPY_INIT_IN_MACRO_ARGUMENT(auto CopyInMacroArg = Obj.reference()); // CHECK-MESSAGES: [[@LINE-1]]:48: warning: the variable 'CopyInMacroArg' is copy-constructed // Ensure fix is not applied. // CHECK-FIXES: auto CopyInMacroArg = Obj.reference() } + +void PositiveLocalCopyConstMethodInvoked() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_1 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_1' of the variable 'orig' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization] + // CHECK-FIXES: const ExpensiveToCopyType& copy_1 = orig; + copy_1.constMethod(); + orig.constMethod(); +} + +void PositiveLocalCopyUsingExplicitCopyCtor() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_2(orig); + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_2' + // CHECK-FIXES: const ExpensiveToCopyType& copy_2(orig); + copy_2.constMethod(); + orig.constMethod(); +} + +void PositiveLocalCopyCopyIsArgument(const ExpensiveToCopyType &orig) { + ExpensiveToCopyType copy_3 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_3' + // CHECK-FIXES: const ExpensiveToCopyType& copy_3 = orig; + copy_3.constMethod(); +} + +void PositiveLocalCopyUsedAsConstRef() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_4 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_4' + // CHECK-FIXES: const ExpensiveToCopyType& copy_4 = orig; + useAsConstReference(orig); +} + +void PositiveLocalCopyTwice() { + ExpensiveToCopyType orig; + ExpensiveToCopyType copy_5 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_5' + // CHECK-FIXES: const ExpensiveToCopyType& copy_5 = orig; + ExpensiveToCopyType copy_6 = copy_5; + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_6' + // CHECK-FIXES: const ExpensiveToCopyType& copy_6 = copy_5; + copy_5.constMethod(); + copy_6.constMethod(); + orig.constMethod(); +} + + +void PositiveLocalCopyWeirdCopy() { + WeirdCopyCtorType orig; + WeirdCopyCtorType weird_1(orig); + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: local copy 'weird_1' + // CHECK-FIXES: const WeirdCopyCtorType& weird_1(orig); + weird_1.constMethod(); + + WeirdCopyCtorType weird_2 = orig; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: local copy 'weird_2' + // CHECK-FIXES: const WeirdCopyCtorType& weird_2 = orig; + weird_2.constMethod(); +} + +void NegativeLocalCopySimpleTypes() { + int i1 = 0; + int i2 = i1; +} + +void NegativeLocalCopyCopyIsModified() { + ExpensiveToCopyType orig; + ExpensiveToCopyType neg_copy_1 = orig; + neg_copy_1.nonConstMethod(); +} + +void NegativeLocalCopyOriginalIsModified() { + ExpensiveToCopyType orig; + ExpensiveToCopyType neg_copy_2 = orig; + orig.nonConstMethod(); +} + +void NegativeLocalCopyUsedAsRefArg() { + ExpensiveToCopyType orig; + ExpensiveToCopyType neg_copy_3 = orig; + mutate(neg_copy_3); +} + +void NegativeLocalCopyUsedAsPointerArg() { + ExpensiveToCopyType orig; + ExpensiveToCopyType neg_copy_4 = orig; + mutate(&neg_copy_4); +} + +void NegativeLocalCopyCopyFromGlobal() { + ExpensiveToCopyType neg_copy_5 = global_expensive_to_copy_type; +} + +void NegativeLocalCopyCopyToStatic() { + ExpensiveToCopyType orig; + static ExpensiveToCopyType neg_copy_6 = orig; +} + +void NegativeLocalCopyNonConstInForLoop() { + ExpensiveToCopyType orig; + for (ExpensiveToCopyType neg_copy_7 = orig; orig.constMethod(); + orig.nonConstMethod()) { + orig.constMethod(); + } +} + +void NegativeLocalCopyWeirdNonCopy() { + WeirdCopyCtorType orig; + WeirdCopyCtorType neg_weird_1(orig, false); + WeirdCopyCtorType neg_weird_2(orig, true); +}
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits