[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
GitOrigin-RevId: ba4411e7c6a5879ce8acf246b0cd03ec738d9d6b
diff --git a/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 514ce6f..c1514ae 100644
--- a/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -83,13 +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().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/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp b/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
index 88b850f..96c7ca8 100644
--- a/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
+++ b/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
@@ -21,6 +21,7 @@
struct ConstInCorrectType {
const ExpensiveToCopy &secretlyMutates() const;
+ const ExpensiveToCopy &operator[](int) const;
};
using NSVTE = ns::ViewType<ExpensiveToCopy>;
@@ -59,12 +60,28 @@
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/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
index 4c469c9..5eac571 100644
--- a/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ b/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 @@
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 @@
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 @@
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 @@
}
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 @@
// 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 @@
}
auto Lambda = []() {
- ExpensiveToCopyType Orig;
+ Container<ExpensiveToCopyType> Orig;
for (const auto &Element : Orig) {
const auto Copy = Element;
Orig.nonConstMethod();
