[clang-tidy] performance-unnecessary-copy-initialization: Restrict UnnecessaryCopyInitialization check to variables initialized from free functions without arguments

[flx]

Summary

This restriction avoids cases where an alias is returned to an argument and which could lead to to a false positive change. Example:

struct S {
  S(const S&);
  void modify();
};

const S& alias(const S& a) {
  return a;
}

void f(S a) {
  const S c = alias(a);
  a.modify();
}

Taking a const reference of the return value of alias would not be safe since a is modified.

A more involved alternative approach would be to inspect each argument passed to these functions for its constness, but this can get complex quickly.

[flx]

The motivating false positive was the free functions std::min. To not limit the usefulness of the check I'll only disable arguments for free functions for now. Updated change coming.

[aaron.ballman]

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

126

I'm not certain I agree with this change, but I'm also not certain I disagree with it. There are more ways to alias than just default arguments (global variables, etc) and the presence of a default argument is not really an indication about its return value in this case. e.g.,

const ExpensiveToCopyType &freeFunctionWithDefaultArg(int i = 12);

(I don't see a reason why = 12 should impact whether use of this function diagnoses or not.)

Perhaps if this was tightened up a bit to care about the type of the default argument it might make sense, but even that gets tricky when you consider base classes, template types, etc. and doesn't cover all of the aliasing cases anyway. Is there an important use case for this behavior?

[flx]

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

126

This was mostly following the logic already applied to the copy constructor where the existing code also allowed default arguments. The idea is to not disallow functions with default arguments that are not overridden.

I'll switch to not allowing any arguments. This will just exclude a few more cases, but better to be safe, until we might be able to verify the constness of all function arguments as well.

[flx]

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

126

This is done now.

[aaron.ballman]

LGTM with some minor nits.

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

122

There's no real need for this change now, so I'd roll back to the previous form (then I don't have to wonder what the 1 means at the call site).

145

Some spurious whitespace changes that can also be rolled back.

[flx]

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

[aaron.ballman]

In D87455#2271582, @flx wrote:

Thank you for the review, Aaron!

I don't have have commit access. Would you mind submitting it?

I've commit on your behalf in 98e07b5596c8692c43770bc4e21a2b19467e35f7, thank you for the patch!

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

 private:
   void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
                                   bool IssueFix, const VarDecl *ObjectArg,
+                                  const CallExpr *InitCallExpr,
                                   ASTContext &Context);
   void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
                               const Stmt &BlockStmt, bool IssueFix,

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

   }
 }
 
+template <typename Call>
+bool hasNonDefaultArgs(const Call &CallExpr, unsigned int StartIndex = 0) {
+  for (unsigned int I = StartIndex; I < CallExpr.getNumArgs(); ++I)
+    if (!CallExpr.getArg(I)->isDefaultArgument())
+      return true;
+  return false;
+}
+
 } // namespace
 
 using namespace ::clang::ast_matchers;

   // called object.
   auto ConstRefReturningMethodCall =
       cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))),
-                        on(declRefExpr(to(varDecl().bind("objectArg")))));
+                        on(declRefExpr(to(varDecl().bind("objectArg")))))
+          .bind("initMethodCall");
   auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
-               unless(callee(cxxMethodDecl())));
+               unless(callee(cxxMethodDecl())))
+          .bind("initFunctionCall");
 
   auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(

   const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
   const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
+  const auto *InitFunctionCall =
+      Result.Nodes.getNodeAs<CallExpr>("initFunctionCall");
+  const auto *InitMethodCall =
+      Result.Nodes.getNodeAs<CallExpr>("initMethodCall");
 
   TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs);
 

   // 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 (hasNonDefaultArgs(*CtorCall, 1))
+    return;
 
   if (OldVar == nullptr) {
     handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+                               InitMethodCall != nullptr ? InitMethodCall
+                                                         : InitFunctionCall,
                                *Result.Context);
   } else {
     handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,

 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
     const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
-    const VarDecl *ObjectArg, ASTContext &Context) {
+    const VarDecl *ObjectArg, const CallExpr *InitCallExpr,
+    ASTContext &Context) {
   bool IsConstQualified = Var.getType().isConstQualified();
-  if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
+  if (hasNonDefaultArgs(*InitCallExpr) ||
+      (!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 "

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

   const ExpensiveToCopyType &reference() const;
   void nonConstMethod();
   bool constMethod() const;
+  const ExpensiveToCopyType &methodWithArg(const ExpensiveToCopyType &) const;
+  const ExpensiveToCopyType &methodWithDefaultArg(int arg = 0) const;
 };
 
 struct TrivialToCopyType {

 ExpensiveToCopyType global_expensive_to_copy_type;
 
 const ExpensiveToCopyType &ExpensiveTypeReference();
+const ExpensiveToCopyType &freeFunctionWithArg(const ExpensiveToCopyType &);
+const ExpensiveToCopyType &freeFunctionWithDefaultArg(
+    const ExpensiveToCopyType *arg = nullptr);
 const TrivialToCopyType &TrivialTypeReference();
 
 void mutate(ExpensiveToCopyType &);

   for (const Element &E : Container()) {
   }
 }
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromMethodWithArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = Orig.methodWithArg(Orig);
+}
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromFreeFunctionWithArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig);
+}
+
+void positiveInitializedFromMethodWithDefaultArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy'
+  // CHECK-FIXES: const ExpensiveToCopyType& Copy = Orig.methodWithDefaultArg();
+}
+
+void negativeInitializedFromMethodWithNonDefaultArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg(5);
+}
+
+void positiveInitializedFromFreeFunctionWithDefaultArg() {
+  const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy'
+  // CHECK-FIXES: const ExpensiveToCopyType& Copy = freeFunctionWithDefaultArg();
+}
+
+void negativeInitialzedFromFreeFunctionWithNonDefaultArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig);
+}