[clang-tidy] UnnecessaryCopyInitialization - Extend to trigger on non-const "this" object argument if it is not modified

[flx]

Summary

Also trigger the check in the following case:

void foo() {

ExpensiveToCopy Obj;
const auto UnnecessaryCopy = Obj.constReference();
Obj.onlyUsedAsConst();

}

i.e. when the object the method is called on is not const but is never modified.

[alexfh]

How many more (in relative numbers) results does this check generate now?

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

45 ↗

(On Diff #56379)

Please enclose inline code snippets in backquotes (this, check(), etc.).

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

133 ↗

(On Diff #56379)

Please include dynamic parts of the message to the CHECK lines (in this case - the variable name).

[flx]

In D20010#427510, @alexfh wrote:

How many more (in relative numbers) results does this check generate now?

147% more :)

[fowles]

LGTM, but I will wait for people who know this stuff better to sign off

[flx]

Friendly ping.

[alexfh]

LG

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h

 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,
-                                  bool IssueFix, ASTContext &Context);
+                                  bool IssueFix, const VarDecl *ObjectArg,
+                                  ASTContext &Context);
   void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
                               const Stmt &BlockStmt, bool IssueFix,
                               ASTContext &Context);
 };
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

 
 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(
+  // Match method call expressions where the `this` argument is only used as
+  // const, this will be checked in `check()` part. 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 ConstRefReturningMethodCall = cxxMemberCallExpr(
       callee(cxxMethodDecl(returns(ConstReference))),
-      on(declRefExpr(to(varDecl(hasType(qualType(ConstOrConstReference)))))));
+      on(declRefExpr(to(varDecl().bind("objectArg")))));
   auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
                unless(callee(cxxMethodDecl())));
 
   auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
                    declStmt(
                        has(varDecl(hasLocalStorage(),
                                    hasType(matchers::isExpensiveToCopy()),
                                    hasInitializer(
                                        cxxConstructExpr(
                                            hasDeclaration(cxxConstructorDecl(
                                                isCopyConstructor())),
                                            hasArgument(0, CopyCtorArg))
                                            .bind("ctorCall")))
                                .bind("newVarDecl"))).bind("declStmt")))
         .bind("blockStmt");
   };
 
   Finder->addMatcher(
       localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall,
-                               ConstRefReturningMethodCallOfConstParam)),
+                               ConstRefReturningMethodCall)),
       this);
 
   Finder->addMatcher(localVarCopiedFrom(declRefExpr(
                          to(varDecl(hasLocalStorage()).bind("oldVarDecl")))),
                      this);
 }
 
 void UnnecessaryCopyInitialization::check(
     const MatchFinder::MatchResult &Result) {
   const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
   const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>("oldVarDecl");
+  const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
   const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
   // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
   // since we cannot place them correctly.
   bool IssueFix =
       Result.Nodes.getNodeAs<DeclStmt>("declStmt")->isSingleDecl() &&
       !NewVar->getLocation().isMacroID();
 
   // 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, IssueFix, *Result.Context);
+    handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+                               *Result.Context);
   } else {
     handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
                            *Result.Context);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
     const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
-    ASTContext &Context) {
+    const VarDecl *ObjectArg, ASTContext &Context) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
     return;
+  if (ObjectArg != nullptr &&
+      !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
+    return;
 
   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 "
                               "const reference but is only used as const "

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-copy-initialization.cpp

 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());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference());
   const ExpensiveToCopyType VarAssigned = ExpensiveTypeReference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES:   const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference();
   const ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference());
 }
 
 void PositiveMethodCallConstReferenceParam(const ExpensiveToCopyType &Obj) {
   const auto AutoAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
   const auto AutoCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
   const ExpensiveToCopyType VarAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
 }
 
 void PositiveMethodCallConstParam(const ExpensiveToCopyType Obj) {
   const auto AutoAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
   const auto AutoCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference());
   const ExpensiveToCopyType VarAssigned = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference());
 }
 
 void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) {
   const auto AutoAssigned = Obj->reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
   // CHECK-FIXES: const auto& AutoAssigned = Obj->reference();
   const auto AutoCopyConstructed(Obj->reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(Obj->reference());
   const ExpensiveToCopyType VarAssigned = Obj->reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj->reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj->reference());
-  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj->reference());
 }
 
 void PositiveLocalConstValue() {
   const ExpensiveToCopyType Obj;
   const auto UnnecessaryCopy = Obj.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = Obj.reference();
 }
 
 void PositiveLocalConstRef() {
   const ExpensiveToCopyType Obj;
   const ExpensiveToCopyType &ConstReference = Obj.reference();
   const auto UnnecessaryCopy = ConstReference.reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = ConstReference.reference();
 }
 
 void PositiveLocalConstPointer() {
   const ExpensiveToCopyType Obj;
   const ExpensiveToCopyType *const ConstPointer = &Obj;
   const auto UnnecessaryCopy = ConstPointer->reference();
-  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy'
   // CHECK-FIXES: const auto& UnnecessaryCopy = ConstPointer->reference();
 }
 
 void NegativeFunctionCallTrivialType() {
   const auto AutoAssigned = TrivialTypeReference();
   const auto AutoCopyConstructed(TrivialTypeReference());
   const TrivialToCopyType VarAssigned = TrivialTypeReference();
   const TrivialToCopyType VarCopyConstructed(TrivialTypeReference());
 }
 
 void NegativeStaticLocalVar(const ExpensiveToCopyType &Obj) {
   static const auto StaticVar = Obj.reference();
 }
 
 void PositiveFunctionCallExpensiveTypeNonConstVariable() {
   auto AutoAssigned = ExpensiveTypeReference();
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'AutoAssigned' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
   // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference();
   auto AutoCopyConstructed(ExpensiveTypeReference());
-  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'AutoCopyConstructed'
   // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference());
   ExpensiveToCopyType VarAssigned = ExpensiveTypeReference();
-  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable 'VarAssigned'
   // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference();
   ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference());
-  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable 'VarCopyConstructed'
   // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference());
 }
 
 void positiveNonConstVarInCodeBlock(const ExpensiveToCopyType &Obj) {
   {
     auto Assigned = Obj.reference();
-    // CHECK-MESSAGES: [[@LINE-1]]:10: warning: the variable
+    // CHECK-MESSAGES: [[@LINE-1]]:10: warning: the variable 'Assigned'
     // CHECK-FIXES: const auto& Assigned = Obj.reference();
     Assigned.reference();
     useAsConstReference(Assigned);
     useByValue(Assigned);
   }
 }
 
 void negativeNonConstVarWithNonConstUse(const ExpensiveToCopyType &Obj) {
void negativeNonConstVarWithNonConstUse(const ExpensiveToCopyType &Obj) {
   }
   {
     auto MutatedByPointer = Obj.reference();
     // CHECK-FIXES: auto MutatedByPointer = Obj.reference();
     mutate(&MutatedByPointer);
   }
 }
 
-void NegativeMethodCallNonConstRef(ExpensiveToCopyType &Obj) {
+void PositiveMethodCallNonConstRefNotModified(ExpensiveToCopyType &Obj) {
   const auto AutoAssigned = Obj.reference();
-  const auto AutoCopyConstructed(Obj.reference());
-  const ExpensiveToCopyType VarAssigned = Obj.reference();
-  const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
 }
 
-void NegativeMethodCallNonConst(ExpensiveToCopyType Obj) {
+void NegativeMethodCallNonConstRefIsModified(ExpensiveToCopyType &Obj) {
   const auto AutoAssigned = Obj.reference();
   const auto AutoCopyConstructed(Obj.reference());
   const ExpensiveToCopyType VarAssigned = Obj.reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+  mutate(&Obj);
+}
+
+void PositiveMethodCallNonConstNotModified(ExpensiveToCopyType Obj) {
+  const auto AutoAssigned = Obj.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = Obj.reference();
 }
 
-void NegativeMethodCallNonConstPointer(ExpensiveToCopyType *const Obj) {
+void NegativeMethodCallNonConstValueArgumentIsModified(ExpensiveToCopyType Obj) {
+  Obj.nonConstMethod();
+  const auto AutoAssigned = Obj.reference();
+}
+
+void PositiveMethodCallNonConstPointerNotModified(ExpensiveToCopyType *const Obj) {
+  const auto AutoAssigned = Obj->reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = Obj->reference();
+  Obj->constMethod();
+}
+
+void NegativeMethodCallNonConstPointerIsModified(ExpensiveToCopyType *const Obj) {
   const auto AutoAssigned = Obj->reference();
   const auto AutoCopyConstructed(Obj->reference());
   const ExpensiveToCopyType VarAssigned = Obj->reference();
   const ExpensiveToCopyType VarCopyConstructed(Obj->reference());
+  mutate(Obj);
 }
 
-void NegativeObjIsNotParam() {
+void PositiveLocalVarIsNotModified() {
+  ExpensiveToCopyType LocalVar;
+  const auto AutoAssigned = LocalVar.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = LocalVar.reference();
+}
+
+void NegativeLocalVarIsModified() {
   ExpensiveToCopyType Obj;
   const auto AutoAssigned = Obj.reference();
-  const auto AutoCopyConstructed(Obj.reference());
-  ExpensiveToCopyType VarAssigned = Obj.reference();
-  ExpensiveToCopyType VarCopyConstructed(Obj.reference());
+  Obj = AutoAssigned;
 }
 
 struct NegativeConstructor {
   NegativeConstructor(const ExpensiveToCopyType &Obj) : Obj(Obj) {}
   ExpensiveToCopyType Obj;
 };
 
 #define UNNECESSARY_COPY_INIT_IN_MACRO_BODY(TYPE)                              \
void NegativeLocalCopyNonConstInForLoop() {
   }
 }
 
 void NegativeLocalCopyWeirdNonCopy() {
   WeirdCopyCtorType orig;
   WeirdCopyCtorType neg_weird_1(orig, false);
   WeirdCopyCtorType neg_weird_2(orig, true);
 }
-
 void WarningOnlyMultiDeclStmt() {
   ExpensiveToCopyType orig;
   ExpensiveToCopyType copy = orig, copy2;
   // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy' of the variable 'orig' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization]
   // CHECK-FIXES: ExpensiveToCopyType copy = orig, copy2;
 }