[clang-tools-extra] r269389 - [clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for decl stmts with single VarDecl.

[Felix Berger via cfe-commits]

Author: flx
Date: Thu May 12 21:47:56 2016
New Revision: 269389

URL: http://llvm.org/viewvc/llvm-project?rev=269389&view=rev
Log:
[clang-tidy] - PerformanceUnnecesaryCopyInitialization - only trigger for decl stmts with single VarDecl.

Summary: This fixes bug: https://llvm.org/bugs/show_bug.cgi?id=27325

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D19865

Modified:
    clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
    clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h
    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=269389&r1=269388&r2=269389&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp Thu May 12 21:47:56 2016
@@ -20,10 +20,6 @@ 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::fixit::changeVarDeclToReference(Var, Context);
   if (!Var.getType().isLocalConstQualified())
     Diagnostic << utils::fixit::changeVarDeclToConst(Var);
@@ -57,14 +53,16 @@ void UnnecessaryCopyInitialization::regi
   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")))
+                   declStmt(
+                       has(varDecl(hasLocalStorage(),
+                                   hasType(matchers::isExpensiveToCopy()),
+                                   hasInitializer(
+                                       cxxConstructExpr(
+                                           hasDeclaration(cxxConstructorDecl(
+                                               isCopyConstructor())),
+                                           hasArgument(0, CopyCtorArg))
+                                           .bind("ctorCall")))
+                               .bind("newVarDecl"))).bind("declStmt")))
         .bind("blockStmt");
   };
 
@@ -84,6 +82,11 @@ void UnnecessaryCopyInitialization::chec
   const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>("oldVarDecl");
   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
@@ -93,14 +96,16 @@ void UnnecessaryCopyInitialization::chec
       return;
 
   if (OldVar == nullptr) {
-    handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Result.Context);
+    handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, *Result.Context);
   } else {
-    handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Result.Context);
+    handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
+                           *Result.Context);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
-    const VarDecl &Var, const Stmt &BlockStmt, ASTContext &Context) {
+    const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
+    ASTContext &Context) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
     return;
@@ -114,12 +119,13 @@ void UnnecessaryCopyInitialization::hand
                               "const reference but is only used as const "
                               "reference; consider making it a const reference")
       << &Var;
-  recordFixes(Var, Context, Diagnostic);
+  if (IssueFix)
+    recordFixes(Var, Context, Diagnostic);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
-    ASTContext &Context) {
+    bool IssueFix, ASTContext &Context) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
       !isOnlyUsedAsConst(OldVar, BlockStmt, Context))
     return;
@@ -128,7 +134,8 @@ void UnnecessaryCopyInitialization::hand
                          "local copy %0 of the variable %1 is never modified; "
                          "consider avoiding the copy")
                     << &NewVar << &OldVar;
-  recordFixes(NewVar, Context, Diagnostic);
+  if (IssueFix)
+    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=269389&r1=269388&r2=269389&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.h Thu May 12 21:47:56 2016
@@ -33,9 +33,10 @@ public:
 
 private:
   void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
-                                  ASTContext &Context);
+                                  bool IssueFix, ASTContext &Context);
   void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
-                              const Stmt &BlockStmt, ASTContext &Context);
+                              const Stmt &BlockStmt, bool IssueFix,
+                              ASTContext &Context);
 };
 
 } // namespace performance

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=269389&r1=269388&r2=269389&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 Thu May 12 21:47:56 2016
@@ -338,3 +338,10 @@ void NegativeLocalCopyWeirdNonCopy() {
   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;
+}