maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[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
This commit is contained in:
Clement Courbet 2021-11-19 16:42:32 +01:00
parent b9fd7247a7
commit ba4411e7c6
3 changed files with 168 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
View File

@ -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) {

17
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
View File

@ -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();
}

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

@ -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();