[clang-tidy] performance-unnecessary-copy-initialization: Search whole function body for variable initializations.

[flx]

Summary

This fixes false positive cases where a reference is initialized outside of a
block statement and then its initializing variable is modified. Another case is
when the looped over container is modified.

[ymandel]

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

109–110

Consider inspecting the DeclContexts instead, which should be much more efficient than searching the entire block. Pass the FunctionDecl as an argument instead of Body, since it is a DeclContext. e.g. const DeclContext &Fun

Then, either

1. Call Fun.containsDecl(InitializingVar), or
2. Search through the contexts yourself; something like:

DeclContext* DC = InitializingVar->getDeclContext(); 
while (DC != nullptr && DC != &Fun)
  DC = DC->getLexicalParent();
if (DC == nullptr)
  // The reference or pointer is not initialized anywhere witin the function. We
  // assume its pointee is not modified then.
  return true;

111

Maybe a bit clearer:
// Search the whole function body, not just the current block statement, for decl statements of the initialization variable.

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

611

typo: Lambda

[flx]

Addressed first round of comments.

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

109–110

Are #1 and #2 equivalent? From the implementation and comment I cannot tell whether #1 would cover cases where the variable is not declared directly in the function, but in child block:

void Fun() {
 {
   var i;
   {
     i.usedHere();
   }  
 } 
}

I'm also reading this as an optimization to more quickly determine whether we can stop here. We still need to find the matches for the next steps, but I think I could then limit matching to the DeclContext I found here. Is this correct? For this I would actually need the DeclContext result from #2.

[ymandel]

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

109–110

A. I think you're right that #2 is more suited to what you need. I wasn't sure, so included both. Agreed that the comments are ambiguous.
B. yes, this is just an optimization. it may be premature for that matter; just that match can be expensive and this seemed a more direct expression of the algorithm.

[flx]

Use more efficient method to check for local variable declaration.

[flx]

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

109–110

I was not able to pass the (possibly more narrow) DeclContext to the match function as scope since match does not support DeclContexts.

I implemented isDeclaredInFunction() which iterates through the decl contexts as you suggested. I'm not sure though whether we should start with VarDecl::getDeclContext() or VarDecl::getLexicalDeclContext()?

While looking at VarDecl::getLexicalDeclContext() I discovered is VarDecl has the following method:

/// Returns true for local variable declarations other than parameters.                                                    
/// Note that this includes static variables inside of functions. It also                                                  
/// includes variables inside blocks.                                                                                      
///                                                                                                                        
///   void foo() { int x; static int y; extern int z; }                                                                    
bool isLocalVarDecl() const;

I think this is exactly what we'd want here. What do you think?

[ymandel]

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

109–110

You mean instead of isDeclaredInFunction? If so -- yes, that seems right. But, if so, are you still planning to bind "declStmt" with the match, or will you replace that with something more direct?

[flx]

Directly examine initializer.

[flx]

Remove now unnecessary FunctionDecl.

[flx]

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

109–110

I was able to examine the`VarDecl.getInit()` expression directly. This completely avoids a search inside the function and he FunctionDecl is also no longer needed. PTAL.

[ymandel]

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

76

This name seems a little off now. maybe rename?

109–110

Nice!

[flx]

Renamed initializer matcher.

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

 //===--- UnnecessaryCopyInitialization.h - clang-tidy------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
 
 #include "../ClangTidyCheck.h"
+#include "clang/AST/Decl.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 // The check detects local variable declarations that are copy initialized with
 // the const reference of a function call or the const reference of a method
 // call whose object is guaranteed to outlive the variable's scope and suggests

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

 //===--- UnnecessaryCopyInitialization.cpp - clang-tidy--------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UnnecessaryCopyInitialization.h"
 #include "../utils/DeclRefExprUtils.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/LexerUtils.h"
 #include "../utils/Matchers.h"
 #include "../utils/OptionsUtils.h"
+#include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 namespace {
 
 using namespace ::clang::ast_matchers;
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
   // Only allow initialization of a const reference from a free function if it
   // has no arguments. Otherwise it could return an alias to one of its
   // arguments and the arguments need to be checked for const use as well.
   return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))),
                   argumentCountIs(0), unless(callee(cxxMethodDecl())))
       .bind(InitFunctionCallId);
 }
 
-AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) {
+AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) {
   auto OldVarDeclRef =
       declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
-  return declStmt(has(varDecl(hasInitializer(
+  return expr(
       anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(),
             ignoringImpCasts(OldVarDeclRef),
-            ignoringImpCasts(unaryOperator(
-                hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef))))))));
+            ignoringImpCasts(unaryOperator(hasOperatorName("&"),
+                                           hasUnaryOperand(OldVarDeclRef)))));
 }
 
 // This checks that the variable itself is only used as const, and also makes
 // sure that it does not reference another variable that could be modified in
 // the BlockStmt. It does this by checking the following:
 // 1. If the variable is neither a reference nor a pointer then the
 // isOnlyUsedAsConst() check is sufficient.
 // 2. If the (reference or pointer) variable is not initialized in a DeclStmt in
static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
   if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
     return false;
 
   QualType T = InitializingVar.getType().getCanonicalType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa<ReferenceType, PointerType>(T))
     return true;
 
-  auto Matches =
-      match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar))))
-                        .bind("declStmt")),
-            BlockStmt, Context);
-  // The reference or pointer is not initialized in the BlockStmt. We assume
-  // its pointee is not modified then.
-  if (Matches.empty())
+  // The reference or pointer is not declared and hence not initialized anywhere
+  // in the function. We assume its pointee is not modified then.
+  if (!InitializingVar.isLocalVarDecl() || !InitializingVar.hasInit()) {
     return true;
+  }
 
-  const auto *Initialization = selectFirst<DeclStmt>("declStmt", Matches);
-  Matches =
-      match(isInitializedFromReferenceToConst(), *Initialization, Context);
+  auto Matches = match(initializerReturnsReferenceToConst(),
+                       *InitializingVar.getInit(), Context);
   // The reference is initialized from a free function without arguments
   // returning a const reference. This is a global immutable object.
   if (selectFirst<CallExpr>(InitFunctionCallId, Matches) != nullptr)
     return true;
   // Check that the object argument is immutable as well.
   if (const auto *OrigVar = selectFirst<VarDecl>(ObjectArgId, Matches))
     return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
   // Check that the old variable we reference is immutable as well.

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

 // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
 
+template <typename T>
+struct Iterator {
+  void operator++();
+  const T &operator*() const;
+  bool operator!=(const Iterator &) const;
+  typedef const T &const_reference;
+};
+
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
   const ExpensiveToCopyType *pointer() const;
+  Iterator<ExpensiveToCopyType> begin() const;
+  Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
 };
 
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 
void positiveUnusedReferenceIsRemoved() {
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference but is never used; consider removing the statement [performance-unnecessary-copy-initialization]
   // CHECK-FIXES-NOT: const auto AutoAssigned = ExpensiveTypeReference();
   // CHECK-FIXES: int i = 0; // Foo bar.
   auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment.
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'TrailingCommentRemoved' is copy-constructed from a const reference but is never used;
   // CHECK-FIXES-NOT: auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment.
   // clang-format on
 }
+
+void negativeloopedOverObjectIsModified() {
+  ExpensiveToCopyType Orig;
+  for (const auto &Element : Orig) {
+    const auto Copy = Element;
+    Orig.nonConstMethod();
+    Copy.constMethod();
+  }
+
+  auto Lambda = []() {
+    ExpensiveToCopyType Orig;
+    for (const auto &Element : Orig) {
+      const auto Copy = Element;
+      Orig.nonConstMethod();
+      Copy.constMethod();
+    }
+  };
+}
+
+void negativeReferenceIsInitializedOutsideOfBlock() {
+  ExpensiveToCopyType Orig;
+  const auto &E2 = Orig;
+  if (1 != 2 * 3) {
+    const auto C2 = E2;
+    Orig.nonConstMethod();
+    C2.constMethod();
+  }
+
+  auto Lambda = []() {
+    ExpensiveToCopyType Orig;
+    const auto &E2 = Orig;
+    if (1 != 2 * 3) {
+      const auto C2 = E2;
+      Orig.nonConstMethod();
+      C2.constMethod();
+    }
+  };
+}