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

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 291030.
flx retitled this revision from "[clang-tidy] 
performance-unnecessary-copy-initialization: Restrict 
UnnecessaryCopyInitialization check to variables initialized from 
methods/functions without arguments " to "[clang-tidy] 
performance-unnecessary-copy-initialization: Restrict 
UnnecessaryCopyInitialization check to variables initialized from free 
functions without arguments ".
flx edited the summary of this revision.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87455/new/

https://reviews.llvm.org/D87455

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


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -23,6 +23,9 @@
 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 &);
@@ -387,3 +390,20 @@
   for (const Element &E : Container()) {
   }
 }
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromFreeFunctionWithArg() {
+  ExpensiveToCopyType Orig;
+  const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig);
+}
+
+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);
+}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -29,6 +29,14 @@
   }
 }
 
+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;
@@ -54,7 +62,8 @@
                         on(declRefExpr(to(varDecl().bind("objectArg")))));
   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(
@@ -96,6 +105,8 @@
   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");
 
   TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs);
 
@@ -108,11 +119,16 @@
   // 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) {
+    // Only allow initialization of a const reference from a free function if it
+    // has no arguments or only default arguments. Otherwise it could return
+    // an alias to one of its arguments and the arguments need to be checked for
+    // const use as well.
+    if (InitFunctionCall != nullptr && hasNonDefaultArgs(*InitFunctionCall))
+      return;
     handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
                                *Result.Context);
   } else {
@@ -127,10 +143,10 @@
   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 "

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits