[clang-tools-extra] r264146 - Add check for unneeded copies of locals
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 23 02:33:07 PDT 2016
Author: hokein
Date: Wed Mar 23 04:33:07 2016
New Revision: 264146
URL: http://llvm.org/viewvc/llvm-project?rev=264146&view=rev
Log:
Add check for unneeded copies of locals
Summary: Extends the UnnecessaryCopyInitialization to detect copies of local variables and parameters that are unneeded.
Patch by Matt Kulukundis!
Reviewers: alexfh, hokein
Subscribers: cfe-commits
Differential Revision: http://reviews.llvm.org/D18149
Modified:
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
Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp?rev=264146&r1=264145&r2=264146&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp Wed Mar 23 04:33:07 2016
@@ -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 @@ void UnnecessaryCopyInitialization::regi
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
Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h?rev=264146&r1=264145&r2=264146&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h Wed Mar 23 04:33:07 2016
@@ -30,6 +30,12 @@ public:
: 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
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst?rev=264146&r1=264145&r2=264146&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst Wed Mar 23 04:33:07 2016
@@ -17,18 +17,21 @@ Example:
.. 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");
}
Modified: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp?rev=264146&r1=264145&r2=264146&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp Wed Mar 23 04:33:07 2016
@@ -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,19 +208,17 @@ struct NegativeConstructor {
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());
@@ -223,3 +226,115 @@ void PositiveMacroArgument(const Expensi
// 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);
+}
More information about the cfe-commits
mailing list