[clang-tools-extra] ba4411e - [clang-tidy] performance-unnecessary-copy-initialization: Fix false negative.

Clement Courbet via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 23 23:14:09 PST 2021


Author: Clement Courbet
Date: 2021-11-24T08:07:21+01:00
New Revision: ba4411e7c6a5879ce8acf246b0cd03ec738d9d6b

URL: https://github.com/llvm/llvm-project/commit/ba4411e7c6a5879ce8acf246b0cd03ec738d9d6b
DIFF: https://github.com/llvm/llvm-project/commit/ba4411e7c6a5879ce8acf246b0cd03ec738d9d6b.diff

LOG: [clang-tidy] performance-unnecessary-copy-initialization: Fix false negative.

`isConstRefReturningMethodCall` should be considering
`CXXOperatorCallExpr` in addition to `CXXMemberCallExpr`. Clang considers
these to be distinct (`CXXOperatorCallExpr` derives from `CallExpr`, not
`CXXMemberCallExpr`), but we don't care in the context of this
check.

This is important because of
`std::vector<Expensive>::operator[](size_t) const`.

Differential Revision: https://reviews.llvm.org/D114249

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
    clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
    clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 514ce6f6e3b87..c1514aed88f70 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -83,13 +83,19 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
   // variable being declared. The assumption is that the const reference being
   // returned either points to a global static variable or to a member of the
   // called object.
-  return cxxMemberCallExpr(
-      callee(cxxMethodDecl(
-                 returns(hasCanonicalType(matchers::isReferenceToConst())))
-                 .bind(MethodDeclId)),
-      on(declRefExpr(to(varDecl().bind(ObjectArgId)))),
-      thisPointerType(namedDecl(
-          unless(matchers::matchesAnyListedName(ExcludedContainerTypes)))));
+  const auto MethodDecl =
+      cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
+          .bind(MethodDeclId);
+  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverType =
+      hasCanonicalType(recordType(hasDeclaration(namedDecl(
+          unless(matchers::matchesAnyListedName(ExcludedContainerTypes))))));
+
+  return expr(anyOf(
+      cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
+                        thisPointerType(ReceiverType)),
+      cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
+                          hasArgument(0, hasType(ReceiverType)))));
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
index 88b850fe2ff82..96c7ca8a460c2 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
@@ -21,6 +21,7 @@ struct ExpensiveToCopy {
 
 struct ConstInCorrectType {
   const ExpensiveToCopy &secretlyMutates() const;
+  const ExpensiveToCopy &operator[](int) const;
 };
 
 using NSVTE = ns::ViewType<ExpensiveToCopy>;
@@ -59,12 +60,28 @@ void excludedConstIncorrectType() {
   E.constMethod();
 }
 
+void excludedConstIncorrectTypeOperator() {
+  ConstInCorrectType C;
+  const auto E = C[42];
+  E.constMethod();
+}
+
 void excludedConstIncorrectTypeAsPointer(ConstInCorrectType *C) {
   const auto E = C->secretlyMutates();
   E.constMethod();
 }
 
+void excludedConstIncorrectTypeAsPointerOperator(ConstInCorrectType *C) {
+  const auto E = (*C)[42];
+  E.constMethod();
+}
+
 void excludedConstIncorrectTypeAsReference(const ConstInCorrectType &C) {
   const auto E = C.secretlyMutates();
   E.constMethod();
 }
+
+void excludedConstIncorrectTypeAsReferenceOperator(const ConstInCorrectType &C) {
+  const auto E = C[42];
+  E.constMethod();
+}

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
index 4c469c966860b..5eac571c2afa7 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -3,11 +3,19 @@
 template <typename T>
 struct Iterator {
   void operator++();
-  const T &operator*() const;
+  T &operator*() const;
   bool operator!=(const Iterator &) const;
   typedef const T &const_reference;
 };
 
+template <typename T>
+struct ConstIterator {
+  void operator++();
+  const T &operator*() const;
+  bool operator!=(const ConstIterator &) const;
+  typedef const T &const_reference;
+};
+
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
@@ -15,8 +23,6 @@ struct ExpensiveToCopyType {
   using ConstRef = const ExpensiveToCopyType &;
   ConstRef referenceWithAlias() const;
   const ExpensiveToCopyType *pointer() const;
-  Iterator<ExpensiveToCopyType> begin() const;
-  Iterator<ExpensiveToCopyType> end() const;
   void nonConstMethod();
   bool constMethod() const;
   template <typename A>
@@ -24,6 +30,21 @@ struct ExpensiveToCopyType {
   operator int() const; // Implicit conversion to int.
 };
 
+template <typename T>
+struct Container {
+  bool empty() const;
+  const T& operator[](int) const;
+  const T& operator[](int);
+  Iterator<T> begin();
+  Iterator<T> end();
+  ConstIterator<T> begin() const;
+  ConstIterator<T> end() const;
+  void nonConstMethod();
+  bool constMethod() const;
+};
+
+using ExpensiveToCopyContainerAlias = Container<ExpensiveToCopyType>;
+
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
@@ -138,6 +159,94 @@ void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) {
   VarCopyConstructed.constMethod();
 }
 
+void PositiveOperatorCallConstReferenceParam(const Container<ExpensiveToCopyType> &C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType> C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlias C) {
+  const auto AutoAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // CHECK-FIXES: const auto& AutoAssigned = C[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // CHECK-FIXES: const auto& AutoCopyConstructed(C[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C[42];
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C[42]);
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C[42]);
+  VarCopyConstructed.constMethod();
+}
+
+void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
+  const auto AutoAssigned = (*C)[42];
+  // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+  // TODO-FIXES: const auto& AutoAssigned = (*C)[42];
+  AutoAssigned.constMethod();
+
+  const auto AutoCopyConstructed((*C)[42]);
+  // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+  // TODO-FIXES: const auto& AutoCopyConstructed((*C)[42]);
+  AutoCopyConstructed.constMethod();
+
+  const ExpensiveToCopyType VarAssigned = C->operator[](42);
+  // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+  // TODO-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
+  VarAssigned.constMethod();
+
+  const ExpensiveToCopyType VarCopyConstructed(C->operator[](42));
+  // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+  // TODO-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
+  VarCopyConstructed.constMethod();
+}
+
 void PositiveLocalConstValue() {
   const ExpensiveToCopyType Obj;
   const auto UnnecessaryCopy = Obj.reference();
@@ -443,21 +552,9 @@ void WarningOnlyMultiDeclStmt() {
 }
 
 class Element {};
-class Container {
-public:
-  class Iterator {
-  public:
-    void operator++();
-    Element operator*();
-    bool operator!=(const Iterator &);
-    WeirdCopyCtorType c;
-  };
-  const Iterator &begin() const;
-  const Iterator &end() const;
-};
 
 void implicitVarFalsePositive() {
-  for (const Element &E : Container()) {
+  for (const Element &E : Container<Element>()) {
   }
 }
 
@@ -621,8 +718,30 @@ void positiveUnusedReferenceIsRemoved() {
   // CHECK-FIXES: // Comments on a new line should not be deleted.
 }
 
-void negativeloopedOverObjectIsModified() {
-  ExpensiveToCopyType Orig;
+void positiveLoopedOverObjectIsConst() {
+  const Container<ExpensiveToCopyType> Orig;
+  for (const auto &Element : Orig) {
+    const auto Copy = Element;
+    // CHECK-MESSAGES: [[@LINE-1]]:16: warning: local copy 'Copy'
+    // CHECK-FIXES: const auto& Copy = Element;
+    Orig.constMethod();
+    Copy.constMethod();
+  }
+
+  auto Lambda = []() {
+    const Container<ExpensiveToCopyType> Orig;
+    for (const auto &Element : Orig) {
+      const auto Copy = Element;
+      // CHECK-MESSAGES: [[@LINE-1]]:18: warning: local copy 'Copy'
+      // CHECK-FIXES: const auto& Copy = Element;
+      Orig.constMethod();
+      Copy.constMethod();
+    }
+  };
+}
+
+void negativeLoopedOverObjectIsModified() {
+  Container<ExpensiveToCopyType> Orig;
   for (const auto &Element : Orig) {
     const auto Copy = Element;
     Orig.nonConstMethod();
@@ -630,7 +749,7 @@ void negativeloopedOverObjectIsModified() {
   }
 
   auto Lambda = []() {
-    ExpensiveToCopyType Orig;
+    Container<ExpensiveToCopyType> Orig;
     for (const auto &Element : Orig) {
       const auto Copy = Element;
       Orig.nonConstMethod();


        


More information about the cfe-commits mailing list