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

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
courbet added reviewers: flx, aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun.
courbet requested review of this revision.
Herald added a project: clang-tools-extra.

`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`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114249

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
@@ -24,6 +24,11 @@
   operator int() const; // Implicit conversion to int.
 };
 
+struct ExpensiveToCopyContainer {
+  const ExpensiveToCopyType &operator[](int) const;
+  const ExpensiveToCopyType &operator[](int);
+};
+
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
@@ -138,6 +143,50 @@
   VarCopyConstructed.constMethod();
 }
 
+void PositiveOperatorCallConstReferenceParam(const ExpensiveToCopyContainer &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 ExpensiveToCopyContainer 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 PositiveLocalConstValue() {
   const ExpensiveToCopyType Obj;
   const auto UnnecessaryCopy = Obj.reference();
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
@@ -83,15 +83,19 @@
   // 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(
-              unless(hasType(qualType(hasCanonicalType(hasDeclaration(namedDecl(
-                  matchers::matchesAnyListedName(ExcludedContainerTypes))))))))
-              .bind(ObjectArgId)))));
+  const auto MethodDecl =
+      cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
+          .bind(MethodDeclId);
+  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverTypeDecl =
+      namedDecl(unless(matchers::matchesAnyListedName(ExcludedContainerTypes)));
+
+  return expr(anyOf(
+      cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
+                        thisPointerType(ReceiverTypeDecl)),
+      cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
+                          hasArgument(0, hasType(recordType(hasDeclaration(
+                                             ReceiverTypeDecl)))))));
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D114249.388505.patch
Type: text/x-patch
Size: 4629 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211119/3d7d5f5d/attachment-0001.bin>