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

[flx]

Summary

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

[alexfh]

Is it a workaround to avoid breaking the code by incorrect fixes?

[flx]

In D19865#419830, @alexfh wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

[flx]

In D19865#419905, @flx wrote:

In D19865#419830, @alexfh wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

Also, all tests still pass. Alex, do you have any particular concern with this approach?

[alexfh]

In D19865#423140, @flx wrote:

In D19865#419905, @flx wrote:

In D19865#419830, @alexfh wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

Also, all tests still pass. Alex, do you have any particular concern with this approach?

Even if we can't easily provide an automated fix (we could teach the check to split declarations, but it might not worth the effort), we could still emit a warning. WDYT?

[flx]

In D19865#423140, @flx wrote:

In D19865#419905, @flx wrote:

In D19865#419830, @alexfh wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

Also, all tests still pass. Alex, do you have any particular concern with this approach?

In D19865#423286, @alexfh wrote:

In D19865#423140, @flx wrote:

In D19865#419905, @flx wrote:

In D19865#419830, @alexfh wrote:

Is it a workaround to avoid breaking the code by incorrect fixes?

Yes. We can't simply change the type of DeclStmt when we only look one of the VarDecls and how it is initialized.

Also, all tests still pass. Alex, do you have any particular concern with this approach?

Even if we can't easily provide an automated fix (we could teach the check to split declarations, but it might not worth the effort), we could still emit a warning. WDYT?

Sounds good. Done. We now still issue the warning, but don't issue fixes when it's not a single decl.

[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,
-                                  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
 } // 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

 
 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::fixit::changeVarDeclToReference(Var, Context);
   if (!Var.getType().isLocalConstQualified())
     Diagnostic << utils::fixit::changeVarDeclToConst(Var);
 }
 
 } // namespace
 
 
auto ConstRefReturningMethodCallOfConstParam = cxxMemberCallExpr(
       on(declRefExpr(to(varDecl(hasType(qualType(ConstOrConstReference)))))));
   auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
                unless(callee(cxxMethodDecl())));
 
   auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
-                   varDecl(hasLocalStorage(),
+                   declStmt(
+                       has(varDecl(hasLocalStorage(),
                                    hasType(matchers::isExpensiveToCopy()),
-                           hasInitializer(cxxConstructExpr(
-                                              hasDeclaration(cxxConstructorDecl(
-                                                  isCopyConstructor())),
-                                              hasArgument(0, CopyCtorArg))
-                                              .bind("ctorCall")))
-                       .bind("newVarDecl")))
+                                   hasInitializer(
+                                       cxxConstructExpr(
+                               .bind("newVarDecl"))).bind("declStmt")))
         .bind("blockStmt");
   };
 
   Finder->addMatcher(
       localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall,
                                ConstRefReturningMethodCallOfConstParam)),
       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 *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, *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;
 
   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 "
                               "reference; consider making it a const reference")
       << &Var;
+  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;
 
   auto Diagnostic = diag(NewVar.getLocation(),
                          "local copy %0 of the variable %1 is never modified; "
                          "consider avoiding the copy")
                     << &NewVar << &OldVar;
+  if (IssueFix)
     recordFixes(NewVar, Context, Diagnostic);
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

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

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;
+}