[clang-tools-extra] [clang-tidy] `doesNotMutateObject`: Handle calls to member functions … (PR #94362)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 4 07:47:57 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: Clement Courbet (legrosbuffle)
<details>
<summary>Changes</summary>
…and operators that have non-const overloads.
This allows `unnecessary-copy-initialization` to warn on more cases.
The common case is a class with a a set of const/non-sconst overloads (e.g. std::vector::operator[]).
```
void F() {
std::vector<Expensive> v;
// ...
const Expensive e = v[i];
}
```
---
Full diff: https://github.com/llvm/llvm-project/pull/94362.diff
5 Files Affected:
- (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp (+10-11)
- (modified) clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp (+156-7)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-1)
- (modified) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp (+27-2)
- (modified) clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp (+33-6)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 9beb185cba929..78a1f9a73687d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -75,16 +75,15 @@ void recordRemoval(const DeclStmt &Stmt, ASTContext &Context,
}
}
-AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
+AST_MATCHER_FUNCTION_P(StatementMatcher, isRefReturningMethodCall,
std::vector<StringRef>, ExcludedContainerTypes) {
// Match method call expressions where the `this` argument is only used as
- // const, this will be checked in `check()` part. This returned const
- // reference is highly likely to outlive the local const reference of the
- // 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.
+ // const, this will be checked in `check()` part. This returned reference is
+ // highly likely to outlive the local const reference of the variable being
+ // declared. The assumption is that the reference being returned either points
+ // to a global static variable or to a member of the called object.
const auto MethodDecl =
- cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
+ cxxMethodDecl(returns(hasCanonicalType(referenceType())))
.bind(MethodDeclId);
const auto ReceiverExpr =
ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
@@ -121,7 +120,7 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
return expr(
anyOf(isConstRefReturningFunctionCall(),
- isConstRefReturningMethodCall(ExcludedContainerTypes),
+ isRefReturningMethodCall(ExcludedContainerTypes),
ignoringImpCasts(OldVarDeclRef),
ignoringImpCasts(unaryOperator(hasOperatorName("&"),
hasUnaryOperand(OldVarDeclRef)))));
@@ -259,9 +258,9 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
.bind("blockStmt");
};
- Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
- isConstRefReturningMethodCall(
- ExcludedContainerTypes))),
+ Finder->addMatcher(LocalVarCopiedFrom(anyOf(
+ isConstRefReturningFunctionCall(),
+ isRefReturningMethodCall(ExcludedContainerTypes))),
this);
Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index a48e45e135681..4ee8a3628061b 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -36,6 +36,111 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
Nodes.insert(Match.getNodeAs<Node>(ID));
}
+// If `D` has a const-qualified overload with otherwise identical
+// ref-qualifiers, returns that overload.
+const CXXMethodDecl *findConstOverload(const CXXMethodDecl &D) {
+ assert(!D.isConst());
+
+ DeclContext::lookup_result lookup_result =
+ D.getParent()->lookup(D.getNameInfo().getName());
+ if (lookup_result.isSingleResult()) {
+ // No overload.
+ return nullptr;
+ }
+ for (const Decl *overload : lookup_result) {
+ const CXXMethodDecl *candidate = dyn_cast<CXXMethodDecl>(overload);
+ if (candidate && !candidate->isDeleted() && candidate->isConst() &&
+ candidate->getRefQualifier() == D.getRefQualifier()) {
+ return candidate;
+ }
+ }
+ return nullptr;
+}
+
+// Returns true if both types refer to the same to the same type,
+// ignoring the const-qualifier.
+bool isSameTypeIgnoringConst(QualType A, QualType B) {
+ A = A.getCanonicalType();
+ B = B.getCanonicalType();
+ A.addConst();
+ B.addConst();
+ return A == B;
+}
+
+// Returns true if both types are pointers or reference to the same type,
+// ignoring the const-qualifier.
+bool pointsToSameTypeIgnoringConst(QualType A, QualType B) {
+ assert(A->isPointerType() || A->isReferenceType());
+ assert(B->isPointerType() || B->isReferenceType());
+ return isSameTypeIgnoringConst(A->getPointeeType(), B->getPointeeType());
+}
+
+// Return true if non-const member function `M` likely does not mutate `*this`.
+//
+// Note that if the member call selects a method/operator `f` that
+// is not const-qualified, then we also consider that the object is
+// not mutated if:
+// - (A) there is a const-qualified overload `cf` of `f` that has
+// the
+// same ref-qualifiers;
+// - (B) * `f` returns a value, or
+// * if `f` returns a `T&`, `cf` returns a `const T&` (up to
+// possible aliases such as `reference` and
+// `const_reference`), or
+// * if `f` returns a `T*`, `cf` returns a `const T*` (up to
+// possible aliases).
+// - (C) the result of the call is not mutated.
+//
+// The assumption that `cf` has the same semantics as `f`.
+// For example:
+// - In `std::vector<T> v; const T t = v[...];`, we consider that
+// expression `v[...]` does not mutate `v` as
+// `T& std::vector<T>::operator[]` has a const overload
+// `const T& std::vector<T>::operator[] const`, and the
+// result expression of type `T&` is only used as a `const T&`;
+// - In `std::map<K, V> m; V v = m.at(...);`, we consider
+// `m.at(...)` to be an immutable access for the same reason.
+// However:
+// - In `std::map<K, V> m; const V v = m[...];`, We consider that
+// `m[...]` mutates `m` as `V& std::map<K, V>::operator[]` does
+// not have a const overload.
+// - In `std::vector<T> v; T& t = v[...];`, we consider that
+// expression `v[...]` mutates `v` as the result is kept as a
+// mutable reference.
+//
+// This function checks (A) ad (B), but the caller should make sure that the
+// object is not mutated through the return value.
+bool isLikelyShallowConst(const CXXMethodDecl &M) {
+ assert(!M.isConst());
+ // The method can mutate our variable.
+
+ // (A)
+ const CXXMethodDecl *ConstOverload = findConstOverload(M);
+ if (ConstOverload == nullptr) {
+ return false;
+ }
+
+ // (B)
+ const QualType CallTy = M.getReturnType().getCanonicalType();
+ const QualType OverloadTy = ConstOverload->getReturnType().getCanonicalType();
+ if (CallTy->isReferenceType()) {
+ if (!(OverloadTy->isReferenceType() &&
+ pointsToSameTypeIgnoringConst(CallTy, OverloadTy))) {
+ return false;
+ }
+ } else if (CallTy->isPointerType()) {
+ if (!(OverloadTy->isPointerType() &&
+ pointsToSameTypeIgnoringConst(CallTy, OverloadTy))) {
+ return false;
+ }
+ } else {
+ if (!isSameTypeIgnoringConst(CallTy, OverloadTy)) {
+ return false;
+ }
+ }
+ return true;
+}
+
// A matcher that matches DeclRefExprs that are used in ways such that the
// underlying declaration is not modified.
// If the declaration is of pointer type, `Indirections` specifies the level
@@ -54,16 +159,15 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
// matches (A).
//
AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
- // We walk up the parents of the DeclRefExpr recursively until we end up on a
- // parent that cannot modify the underlying object. There are a few kinds of
- // expressions:
- // - Those that cannot be used to mutate the underlying object. We can stop
+ // We walk up the parents of the DeclRefExpr recursively. There are a few
+ // kinds of expressions:
+ // - Those that cannot be used to mutate the underlying variable. We can stop
// recursion there.
- // - Those that can be used to mutate the underlying object in analyzable
+ // - Those that can be used to mutate the underlying variable in analyzable
// ways (such as taking the address or accessing a subobject). We have to
// examine the parents.
// - Those that we don't know how to analyze. In that case we stop there and
- // we assume that they can mutate the underlying expression.
+ // we assume that they can modify the expression.
struct StackEntry {
StackEntry(const Expr *E, int Indirections)
@@ -90,7 +194,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
assert(Ty->isPointerType());
Ty = Ty->getPointeeType().getCanonicalType();
}
- if (Ty.isConstQualified())
+ if (Ty->isVoidType() || Ty.isConstQualified())
continue;
// Otherwise we have to look at the parents to see how the expression is
@@ -159,11 +263,56 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
// The method call cannot mutate our variable.
continue;
}
+ if (isLikelyShallowConst(*Method)) {
+ // We still have to check that the object is not modified through
+ // the method's return value (C).
+ const auto MemberParents = Ctx.getParents(*Member);
+ assert(MemberParents.size() == 1);
+ const CallExpr *Call = MemberParents[0].get<CallExpr>();
+ // If `o` is an object of class type and `f` is a member function,
+ // then `o.f` has to be used as part of a call expression.
+ assert(Call != nullptr && "member function has to be called");
+ Stack.emplace_back(
+ Call,
+ Method->getReturnType().getCanonicalType()->isPointerType()
+ ? 1
+ : 0);
+ continue;
+ }
return false;
}
Stack.emplace_back(Member, 0);
continue;
}
+ if (const auto *const OpCall = dyn_cast<CXXOperatorCallExpr>(P)) {
+ // Operator calls have function call syntax. The `*this` parameter
+ // is the first parameter.
+ if (OpCall->getNumArgs() == 0 || OpCall->getArg(0) != Entry.E) {
+ return false;
+ }
+ const auto *const Method =
+ dyn_cast<CXXMethodDecl>(OpCall->getDirectCallee());
+
+ if (Method == nullptr) {
+ // This is not a member operator. Typically, a friend operator. These
+ // are handled like function calls.
+ return false;
+ }
+
+ if (Method->isConst() || Method->isStatic()) {
+ continue;
+ }
+ if (isLikelyShallowConst(*Method)) {
+ // We still have to check that the object is not modified through
+ // the operator's return value (C).
+ Stack.emplace_back(
+ OpCall,
+ Method->getReturnType().getCanonicalType()->isPointerType() ? 1
+ : 0);
+ continue;
+ }
+ return false;
+ }
if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
switch (Op->getOpcode()) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 33b65caf2b02c..b8392d5b2d58e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -362,8 +362,10 @@ Changes in existing checks
- Improved :doc:`performance-unnecessary-copy-initialization
<clang-tidy/checks/performance/unnecessary-copy-initialization>` check by
detecting more cases of constant access. In particular, pointers can be
- analyzed, se the check now handles the common patterns
+ analyzed, so the check now handles the common patterns
`const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`.
+ Calls to mutable function where there exists a `const` overload are also
+ handled.
- Improved :doc:`readability-avoid-return-with-void-value
<clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding
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 92625cc1332e2..f259552dc8f1d 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
@@ -32,6 +32,9 @@ struct ExpensiveToCopyType {
template <typename T>
struct Container {
+ using reference = T&;
+ using const_reference = const T&;
+
bool empty() const;
const T& operator[](int) const;
const T& operator[](int);
@@ -42,8 +45,8 @@ struct Container {
void nonConstMethod();
bool constMethod() const;
- const T& at(int) const;
- T& at(int);
+ reference at(int) const;
+ const_reference at(int);
};
@@ -207,6 +210,28 @@ void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType> C)
VarCopyConstructed.constMethod();
}
+void PositiveOperatorValueParam(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.at(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C.at(42);
+ VarAssigned.constMethod();
+
+ const ExpensiveToCopyType VarCopyConstructed(C.at(42));
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C.at(42));
+ VarCopyConstructed.constMethod();
+}
+
void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlias C) {
const auto AutoAssigned = C[42];
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
index 3d9f51e2e17b0..af314d125d414 100644
--- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -59,6 +59,9 @@ template <int Indirections> void RunTest(StringRef Snippet) {
void operator[](int);
void operator[](int) const;
+ int& at(int);
+ const int& at(int) const;
+
bool operator==(const S&) const;
int int_member;
@@ -161,9 +164,11 @@ TEST(ConstReferenceDeclRefExprsTest, ConstRefVar) {
useIntConstRef(/*const*/target.int_member);
useIntPtr(/*const*/target.ptr_member);
useIntConstPtr(&/*const*/target.int_member);
+ (void)/*const*/target.at(3);
const S& const_target_ref = /*const*/target;
const S* const_target_ptr = &/*const*/target;
+ (void)/*const*/target.at(3);
}
)");
}
@@ -187,9 +192,9 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
/*const*/target.staticMethod();
target.nonConstMethod();
/*const*/target(ConstTag{});
- target[42];
+ /*const*/target[42];
/*const*/target(ConstTag{});
- target(NonConstTag{});
+ /*const*/target(NonConstTag{});
useRef(target);
usePtr(&target);
useConstRef((/*const*/target));
@@ -211,6 +216,12 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
const S& const_target_ref = /*const*/target;
const S* const_target_ptr = &/*const*/target;
S* target_ptr = ⌖
+
+ (void)/*const*/target.at(3);
+ ++target.at(3);
+ const int civ = /*const*/target.at(3);
+ const int& cir = /*const*/target.at(3);
+ int& ir = target.at(3);
}
)");
}
@@ -227,7 +238,7 @@ TEST(ConstReferenceDeclRefExprsTest, RefVar) {
/*const*/target.staticMethod();
target.nonConstMethod();
/*const*/target(ConstTag{});
- target[42];
+ /*const*/target[42];
useConstRef((/*const*/target));
(/*const*/target).constMethod();
(void)(/*const*/target == /*const*/target);
@@ -249,6 +260,12 @@ TEST(ConstReferenceDeclRefExprsTest, RefVar) {
const S& const_target_ref = /*const*/target;
const S* const_target_ptr = &/*const*/target;
S* target_ptr = ⌖
+
+ (void)/*const*/target.at(3);
+ ++target.at(3);
+ const int civ = /*const*/target.at(3);
+ const int& cir = /*const*/target.at(3);
+ int& ir = target.at(3);
}
)");
}
@@ -266,8 +283,8 @@ TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
/*const*/target->staticMethod();
target->nonConstMethod();
(*/*const*/target)(ConstTag{});
- (*target)[42];
- target->operator[](42);
+ (*/*const*/target)[42];
+ /*const*/target->operator[](42);
useConstRef((*/*const*/target));
(/*const*/target)->constMethod();
(void)(*/*const*/target == */*const*/target);
@@ -284,7 +301,13 @@ TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
const S& const_target_ref = */*const*/target;
const S* const_target_ptr = /*const*/target;
- S* target_ptr = target; // FIXME: we could chect const usage of `target_ptr`.
+ S* target_ptr = target; // FIXME: we could chect const usage of `target_ptr`
+
+ (void)/*const*/target->at(3);
+ ++target->at(3);
+ const int civ = /*const*/target->at(3);
+ const int& cir = /*const*/target->at(3);
+ int& ir = target->at(3);
}
)");
}
@@ -319,6 +342,10 @@ TEST(ConstReferenceDeclRefExprsTest, ConstPtrVar) {
const S& const_target_ref = */*const*/target;
const S* const_target_ptr = /*const*/target;
+
+ (void)/*const*/target->at(3);
+ const int civ = /*const*/target->at(3);
+ const int& cir = /*const*/target->at(3);
}
)");
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/94362
More information about the cfe-commits
mailing list