Add check for unneeded copies of locals

[fowles]

Summary

Extends the UnnecessaryCopyInitialization to detect copies of local variables and parameters that are unneeded.

[alexfh]

Please always add 'cfe-commits' to Subscribers when you send a patch for review (I've added it now).

Substantial comments later.

[alexfh]

Thank you for the patch! Looks mostly good, a few style nits.

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

36

No need for clang::tidy:: here.

58

I guess, there's no need for ast_matchers:: here.

128

nit: I'd use a semicolon instead of the comma here, like in other clang-tidy messages.

clang-tidy/performance/UnnecessaryCopyInitialization.h

29

LLVM style is to put *'s and &'s to the variable. Looks like your clang-format didn't understand it should use LLVM style. You might want to explicitly specify -style=llvm next time.

[hokein]

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

102

I prefer to move the anonymous namespace under namespace performance { statement.

test/clang-tidy/performance-unnecessary-copy-initialization.cpp

232

It would be nice to add a test case with copy_2 like:

ExpensiveToCopyType orig;
ExpensiveToCopyType copy_1 = orig;
ExpensiveToCopyType copy_2 = copy_1;
copy_1.constMethod();
copy_2.constMethod();
orig.constMethod();

[hokein]

LGTM.

[alexfh]

LG

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

21

nit: Alternatively, you could return llvm::Optional<FixItHint> and apply it in the caller. Might result in a bit better separation of responsibilities.

[fowles]

rebased to latest. Can someone submit this for me I don't have commit bits.

[fowles]

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

21

I am inclined to just leave it as is for the moment, this is already isolated code that doesn't escape the current file.

[hokein]

@fowles, I have commit the patch for you.

[fowles]

thank you!

clang-tidy/performance/UnnecessaryCopyInitialization.h

 // call whose object is guaranteed to outlive the variable's scope and suggests
 // to use a const reference.
 //
 // The check currently only understands a subset of variables that are
 // guaranteed to outlive the const reference returned, namely: const variables,
 // const references, and const pointers to const.
 class UnnecessaryCopyInitialization : public ClangTidyCheck {
 public:
   UnnecessaryCopyInitialization(StringRef Name, ClangTidyContext *Context)
       : 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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

 
 #include "../utils/DeclRefExprUtils.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/Matchers.h"
 
 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.
   // The assumption is that the const reference being returned either points
   // to a global static variable or to a member of the called object.
   auto ConstRefReturningMethodCallOfConstParam = cxxMemberCallExpr(
       callee(cxxMethodDecl(returns(ConstReference))),
       on(declRefExpr(to(varDecl(hasType(qualType(ConstOrConstReference)))))));
   auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
                unless(callee(cxxMethodDecl())));
-  Finder->addMatcher(
-      compoundStmt(
+
+  auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) {
+    return compoundStmt(
                forEachDescendant(
-              varDecl(
-                  hasLocalStorage(), hasType(matchers::isExpensiveToCopy()),
+                   varDecl(hasLocalStorage(),
+                           hasType(matchers::isExpensiveToCopy()),
                            hasInitializer(cxxConstructExpr(
-                      hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
-                      hasArgument(
-                          0, anyOf(ConstRefReturningFunctionCall,
-                                   ConstRefReturningMethodCallOfConstParam)))))
-                  .bind("varDecl"))).bind("blockStmt"),
+                                              hasDeclaration(cxxConstructorDecl(
+                                                  isCopyConstructor())),
+                                              hasArgument(0, CopyCtorArg))
+                                              .bind("ctorCall")))
+                       .bind("newVarDecl")))
+        .bind("blockStmt");
+  };
+
+  Finder->addMatcher(
+      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
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

 only used as a const, i.e. only const methods or operators are invoked on it, or
 it is used as const reference or value argument in constructors or function
 calls.
 
 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");
   }

test/clang-tidy/performance-unnecessary-copy-initialization.cpp

 // 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);
 
 void PositiveFunctionCall() {
   const auto AutoAssigned = ExpensiveTypeReference();
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
   // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference();
   const auto AutoCopyConstructed(ExpensiveTypeReference());
void NegativeObjIsNotParam() {
   ExpensiveToCopyType VarCopyConstructed(Obj.reference());
 }
 
 struct NegativeConstructor {
   NegativeConstructor(const ExpensiveToCopyType &Obj) : Obj(Obj) {}
   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);
+}