[clang-tools-extra] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)
Clement Courbet via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 26 00:38:48 PST 2024
https://github.com/legrosbuffle updated https://github.com/llvm/llvm-project/pull/82617
>From b2b98e9594b224f471f88924b8b060bee06de483 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Wed, 21 Feb 2024 09:15:22 +0000
Subject: [PATCH 1/3] [clang-tidy] Add support for determining constness of
more expressions.
This uses a more systematic approach for determining whcich `DeclRefExpr`s mutate
the underlying object: Instead of using a few matchers, we walk up the
AST until we find a parent that we can prove cannot change the
underlying object.
This allows us to handle most address taking and dereference, bindings
to value and const& variables, and track constness of pointee
(see changes in DeclRefExprUtilsTest.cpp).
This allows supporting more patterns in `performance-unnecessary-copy-initialization`.
Those two patterns are relatively common:
```
const auto e = (*vector_ptr)[i]
```
and
```
const auto e = vector_ptr->at(i);
```
In our codebase, we have around 25% additional findings from
`performance-unnecessary-copy-initialization` with this change.
I did not see any additional false positives.
---
.../UnnecessaryCopyInitialization.cpp | 27 +-
.../clang-tidy/utils/DeclRefExprUtils.cpp | 212 +++++++++++---
.../clang-tidy/utils/DeclRefExprUtils.h | 33 ++-
clang-tools-extra/docs/ReleaseNotes.rst | 6 +
.../unnecessary-copy-initialization.cpp | 27 +-
.../clang-tidy/DeclRefExprUtilsTest.cpp | 274 +++++++++++-------
6 files changed, 396 insertions(+), 183 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
const auto MethodDecl =
cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
.bind(MethodDeclId);
- const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+ const auto ReceiverExpr =
+ ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
+ const auto OnExpr = anyOf(
+ // Direct reference to `*this`: `a.f()` or `a->f()`.
+ ReceiverExpr,
+ // Access through dereference, typically used for `operator[]`: `(*a)[3]`.
+ unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
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)))));
+ return expr(
+ anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+ thisPointerType(ReceiverType)),
+ cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+ hasArgument(0, hasType(ReceiverType)))));
}
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
static bool isInitializingVariableImmutable(
const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context,
const std::vector<StringRef> &ExcludedContainerTypes) {
- if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+ QualType T = InitializingVar.getType().getCanonicalType();
+ if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
return false;
- QualType T = InitializingVar.getType().getCanonicalType();
// The variable is a value type and we know it is only used as const. Safe
// to reference it and avoid the copy.
if (!isa<ReferenceType, PointerType>(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, *Result.Context);
const bool IsVarOnlyUsedAsConst =
- isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+ isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
+ // `NewVar` is always of non-pointer type.
+ 0);
const CheckContext Context{
NewVar, BlockStmt, VarDeclStmt, *Result.Context,
IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 2d73179150e8b8..663691c519b8e9 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -10,7 +10,9 @@
#include "Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <cassert>
namespace clang::tidy::utils::decl_ref_expr {
@@ -34,69 +36,185 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
Nodes.insert(Match.getNodeAs<Node>(ID));
}
+// 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
+// of indirection of the object whose mutations we are tracking.
+//
+// For example, given:
+// ```
+// int i;
+// int* p;
+// p = &i; // (A)
+// *p = 3; // (B)
+// ```
+//
+// `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(0))` matches
+// (B), but `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(1))`
+// 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
+ // recursion there.
+ // - Those that can be used to mutate the underlying object 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.
+
+ struct StackEntry {
+ StackEntry(const Expr *E, int Indirections)
+ : E(E), Indirections(Indirections) {}
+ // The expression to analyze.
+ const Expr *E;
+ // The number of pointer indirections of the object being tracked (how
+ // many times an address was taken).
+ int Indirections;
+ };
+
+ llvm::SmallVector<StackEntry, 4> Stack;
+ Stack.emplace_back(&Node, Indirections);
+ auto &Ctx = Finder->getASTContext();
+
+ while (!Stack.empty()) {
+ const StackEntry Entry = Stack.back();
+ Stack.pop_back();
+
+ // If the expression type is const-qualified at the appropriate indirection
+ // level then we can not mutate the object.
+ QualType Ty = Entry.E->getType().getCanonicalType();
+ for (int I = 0; I < Entry.Indirections; ++I) {
+ assert(Ty->isPointerType());
+ Ty = Ty->getPointeeType().getCanonicalType();
+ }
+ if (Ty.isConstQualified()) {
+ continue;
+ }
+
+ // Otherwise we have to look at the parents to see how the expression is
+ // used.
+ const auto Parents = Ctx.getParents(*Entry.E);
+ // Note: most nodes have a single parents, but there exist nodes that have
+ // several parents, such as `InitListExpr` that have semantic and syntactic
+ // forms.
+ for (const auto &Parent : Parents) {
+ if (Parent.get<CompoundStmt>()) {
+ // Unused block-scope statement.
+ continue;
+ }
+ const Expr *const P = Parent.get<Expr>();
+ if (P == nullptr) {
+ // `Parent` is not an expr (e.g. a `VarDecl`).
+ // The case of binding to a `const&` or `const*` variable is handled by
+ // the fact that there is going to be a `NoOp` cast to const below the
+ // `VarDecl`, so we're not even going to get there.
+ // The case of copying into a value-typed variable is handled by the
+ // rvalue cast.
+ // This triggers only when binding to a mutable reference/ptr variable.
+ // FIXME: When we take a mutable reference we could keep checking the
+ // new variable for const usage only.
+ return false;
+ }
+ // Cosmetic nodes.
+ if (isa<ParenExpr>(P) || isa<MaterializeTemporaryExpr>(P)) {
+ Stack.emplace_back(P, Entry.Indirections);
+ continue;
+ }
+ if (const auto *const Cast = dyn_cast<CastExpr>(P)) {
+ switch (Cast->getCastKind()) {
+ // NoOp casts are used to add `const`. We'll check whether adding that
+ // const prevents modification when we process the cast.
+ case CK_NoOp:
+ // These do nothing w.r.t. to mutability.
+ case CK_BaseToDerived:
+ case CK_DerivedToBase:
+ case CK_UncheckedDerivedToBase:
+ case CK_Dynamic:
+ case CK_BaseToDerivedMemberPointer:
+ case CK_DerivedToBaseMemberPointer:
+ Stack.emplace_back(Cast, Entry.Indirections);
+ continue;
+ case CK_ToVoid:
+ case CK_PointerToBoolean:
+ // These do not mutate the underlying variable.
+ continue;
+ case CK_LValueToRValue: {
+ // An rvalue is immutable.
+ if (Entry.Indirections == 0) {
+ continue;
+ }
+ Stack.emplace_back(Cast, Entry.Indirections);
+ continue;
+ }
+ default:
+ // Bail out on casts that we cannot analyze.
+ return false;
+ }
+ }
+ if (const auto *const Member = dyn_cast<MemberExpr>(P)) {
+ if (const auto *const Method =
+ dyn_cast<CXXMethodDecl>(Member->getMemberDecl())) {
+ if (!Method->isConst()) {
+ // The method can mutate our variable.
+ return false;
+ }
+ continue;
+ }
+ Stack.emplace_back(Member, 0);
+ continue;
+ }
+ if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
+ switch (Op->getOpcode()) {
+ case UO_AddrOf:
+ Stack.emplace_back(Op, Entry.Indirections + 1);
+ continue;
+ case UO_Deref:
+ assert(Entry.Indirections > 0);
+ Stack.emplace_back(Op, Entry.Indirections - 1);
+ continue;
+ default:
+ // Bail out on unary operators that we cannot analyze.
+ return false;
+ }
+ }
+
+ // Assume any other expression can modify the underlying variable.
+ return false;
+ }
+ }
+
+ // No parent can modify the variable.
+ return true;
+}
+
} // namespace
-// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
-// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
- ASTContext &Context) {
- auto DeclRefToVar =
- declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
- auto MemberExprOfVar = memberExpr(hasObjectExpression(DeclRefToVar));
- auto DeclRefToVarOrMemberExprOfVar =
- stmt(anyOf(DeclRefToVar, MemberExprOfVar));
- auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
- // Match method call expressions where the variable is referenced as the this
- // implicit object argument and operator call expression for member operators
- // where the variable is the 0-th argument.
- auto Matches = match(
- findAll(expr(anyOf(
- cxxMemberCallExpr(ConstMethodCallee,
- on(DeclRefToVarOrMemberExprOfVar)),
- cxxOperatorCallExpr(ConstMethodCallee,
- hasArgument(0, DeclRefToVarOrMemberExprOfVar))))),
- Stmt, Context);
+ ASTContext &Context, int Indirections) {
+ auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl))),
+ doesNotMutateObject(Indirections))
+ .bind("declRef")),
+ Stmt, Context);
SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
extractNodesByIdTo(Matches, "declRef", DeclRefs);
- auto ConstReferenceOrValue =
- qualType(anyOf(matchers::isReferenceToConst(),
- unless(anyOf(referenceType(), pointerType(),
- substTemplateTypeParmType()))));
- auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
- ConstReferenceOrValue,
- substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
- auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
- DeclRefToVarOrMemberExprOfVar,
- parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
- Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- // References and pointers to const assignments.
- Matches = match(
- findAll(declStmt(has(varDecl(
- hasType(qualType(matchers::isReferenceToConst())),
- hasInitializer(ignoringImpCasts(DeclRefToVarOrMemberExprOfVar)))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- Matches = match(findAll(declStmt(has(varDecl(
- hasType(qualType(matchers::isPointerToConst())),
- hasInitializer(ignoringImpCasts(unaryOperator(
- hasOperatorName("&"),
- hasUnaryOperand(DeclRefToVarOrMemberExprOfVar)))))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
+
return DeclRefs;
}
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
- ASTContext &Context) {
+ ASTContext &Context, int Indirections) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
// CXXConstructExprs where the loop variable is used as argument to a const
// reference parameter.
// If the difference is empty it is safe for the loop variable to be a const
// reference.
auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
- auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
+ auto ConstReferenceDeclRefs =
+ constReferenceDeclRefExprs(Var, Stmt, Context, Indirections);
return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
}
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
index 2c16d95408cf68..8361b9d89ed268 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
@@ -15,15 +15,6 @@
namespace clang::tidy::utils::decl_ref_expr {
-/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
-/// do not modify it.
-///
-/// Returns ``true`` if only const methods or operators are called on the
-/// variable or the variable is a const reference or value argument to a
-/// ``callExpr()``.
-bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
- ASTContext &Context);
-
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
@@ -34,9 +25,31 @@ allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
+///
+/// If ``VarDecl`` is of pointer type, ``Indirections`` specifies the level
+/// of indirection of the object whose mutations we are tracking.
+///
+/// For example, given:
+/// ```
+/// int i;
+/// int* p;
+/// p = &i; // (A)
+/// *p = 3; // (B)
+/// ```
+///
+/// - `constReferenceDeclRefExprs(P, Stmt, Context, 0)` returns the reference
+// to `p` in (B): the pointee is modified, but the pointer is not;
+/// - `constReferenceDeclRefExprs(P, Stmt, Context, 1)` returns the reference
+// to `p` in (A): the pointee is modified, but the pointer is not;
llvm::SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
- ASTContext &Context);
+ ASTContext &Context, int Indirections);
+
+/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
+/// do not modify it.
+/// See `constReferenceDeclRefExprs` for the meaning of ``Indirections``.
+bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
+ ASTContext &Context, int Indirections);
/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor
/// call expression within ``Decl``.
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a0b9fcfe0d7774..79eb777afee575 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -179,6 +179,12 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-override>` check to also remove any trailing
whitespace when deleting the ``virtual`` keyword.
+- 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
+ `const auto e = (*vector_ptr)[i]` and `const auto e = vector_ptr->at(i);`.
+
- Improved :doc:`readability-implicit-bool-conversion
<clang-tidy/checks/readability/implicit-bool-conversion>` check to provide
valid fix suggestions for ``static_cast`` without a preceding space and
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 049ba64d6aedeb..92625cc1332e28 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
@@ -41,6 +41,10 @@ struct Container {
ConstIterator<T> end() const;
void nonConstMethod();
bool constMethod() const;
+
+ const T& at(int) const;
+ T& at(int);
+
};
using ExpensiveToCopyContainerAlias = Container<ExpensiveToCopyType>;
@@ -225,25 +229,25 @@ void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlia
VarCopyConstructed.constMethod();
}
-void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
+void PositiveOperatorCallConstPtrParam(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];
+ // 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]);
- // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
- // TODO-FIXES: 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->operator[](42);
- // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
- // TODO-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
+ 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->operator[](42));
- // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
- // TODO-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
+ 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();
}
@@ -876,3 +880,4 @@ void negativeNonConstMemberExpr() {
mutate(&Copy.Member);
}
}
+
diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
index a653b11faad282..4c9e81ea0f61ac 100644
--- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -12,6 +12,7 @@ namespace tidy {
namespace {
using namespace clang::ast_matchers;
+template <int Indirections>
class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
public:
ConstReferenceDeclRefExprsTransform(StringRef CheckName,
@@ -27,7 +28,7 @@ class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
using utils::decl_ref_expr::constReferenceDeclRefExprs;
const auto const_decrefexprs = constReferenceDeclRefExprs(
*D, *cast<FunctionDecl>(D->getDeclContext())->getBody(),
- *Result.Context);
+ *Result.Context, Indirections);
for (const DeclRefExpr *const Expr : const_decrefexprs) {
assert(Expr);
@@ -40,7 +41,7 @@ class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
namespace test {
-void RunTest(StringRef Snippet) {
+template <int Indirections> void RunTest(StringRef Snippet) {
StringRef CommonCode = R"(
struct ConstTag{};
@@ -59,6 +60,9 @@ void RunTest(StringRef Snippet) {
bool operator==(const S&) const;
int int_member;
+ // We consider a mutation of the `*ptr_member` to be a const use of
+ // `*this`. This is consistent with the semantics of `const`-qualified
+ // methods, which prevent modifying `ptr_member` but not `*ptr_member`.
int* ptr_member;
};
@@ -92,69 +96,88 @@ void RunTest(StringRef Snippet) {
llvm::SmallVector<StringRef, 1> Parts;
StringRef(Code).split(Parts, "/*const*/");
- EXPECT_EQ(Code, runCheckOnCode<ConstReferenceDeclRefExprsTransform>(
- join(Parts, "")));
+ EXPECT_EQ(Code,
+ runCheckOnCode<ConstReferenceDeclRefExprsTransform<Indirections>>(
+ join(Parts, "")));
}
TEST(ConstReferenceDeclRefExprsTest, ConstValueVar) {
- RunTest(R"(
+ RunTest<0>(R"(
void f(const S target) {
useVal(/*const*/target);
useConstRef(/*const*/target);
- useConstPtr(&target);
- useConstPtrConstRef(&target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
/*const*/target.constMethod();
/*const*/target(ConstTag{});
/*const*/target[42];
useConstRef((/*const*/target));
(/*const*/target).constMethod();
(void)(/*const*/target == /*const*/target);
- (void)target;
- (void)⌖
- (void)*⌖
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ /*const*/target;
S copy1 = /*const*/target;
S copy2(/*const*/target);
+ /*const*/target.int_member;
useInt(/*const*/target.int_member);
useIntConstRef(/*const*/target.int_member);
- useIntPtr(target.ptr_member);
- useIntConstPtr(&target.int_member);
+ useIntPtr(/*const*/target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+
+ const S& const_target_ref = /*const*/target;
+ const S* const_target_ptr = &/*const*/target;
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, ConstRefVar) {
- RunTest(R"(
+ RunTest<0>(R"(
void f(const S& target) {
useVal(/*const*/target);
useConstRef(/*const*/target);
- useConstPtr(&target);
- useConstPtrConstRef(&target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
/*const*/target.constMethod();
/*const*/target(ConstTag{});
/*const*/target[42];
useConstRef((/*const*/target));
(/*const*/target).constMethod();
(void)(/*const*/target == /*const*/target);
- (void)target;
- (void)⌖
- (void)*⌖
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ /*const*/target;
S copy1 = /*const*/target;
S copy2(/*const*/target);
+ /*const*/target.int_member;
useInt(/*const*/target.int_member);
useIntConstRef(/*const*/target.int_member);
- useIntPtr(target.ptr_member);
- useIntConstPtr(&target.int_member);
+ useIntPtr(/*const*/target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+
+ const S& const_target_ref = /*const*/target;
+ const S* const_target_ptr = &/*const*/target;
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, DEBUGREMOVEME) {
+ RunTest<0>(R"(
+ void f(S target, const S& other) {
+ S* target_ptr = ⌖
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
- RunTest(R"(
+ RunTest<0>(R"(
void f(S target, const S& other) {
useConstRef(/*const*/target);
useVal(/*const*/target);
- useConstPtr(&target);
- useConstPtrConstRef(&target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
/*const*/target.constMethod();
target.nonConstMethod();
/*const*/target(ConstTag{});
@@ -167,26 +190,33 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
(/*const*/target).constMethod();
(void)(/*const*/target == /*const*/target);
(void)(/*const*/target == other);
- (void)target;
- (void)⌖
- (void)*⌖
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ /*const*/target;
S copy1 = /*const*/target;
S copy2(/*const*/target);
+ /*const*/target.int_member;
useInt(/*const*/target.int_member);
useIntConstRef(/*const*/target.int_member);
- useIntPtr(target.ptr_member);
- useIntConstPtr(&target.int_member);
+ useIntPtr(/*const*/target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+
+ const S& const_target_ref = /*const*/target;
+ const S* const_target_ptr = &/*const*/target;
+ S* target_ptr = ⌖
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, RefVar) {
- RunTest(R"(
+ RunTest<0>(R"(
void f(S& target) {
useVal(/*const*/target);
+ usePtr(&target);
useConstRef(/*const*/target);
- useConstPtr(&target);
- useConstPtrConstRef(&target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
/*const*/target.constMethod();
target.nonConstMethod();
/*const*/target(ConstTag{});
@@ -194,118 +224,150 @@ TEST(ConstReferenceDeclRefExprsTest, RefVar) {
useConstRef((/*const*/target));
(/*const*/target).constMethod();
(void)(/*const*/target == /*const*/target);
- (void)target;
- (void)⌖
- (void)*⌖
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ /*const*/target;
S copy1 = /*const*/target;
S copy2(/*const*/target);
+ /*const*/target.int_member;
useInt(/*const*/target.int_member);
useIntConstRef(/*const*/target.int_member);
- useIntPtr(target.ptr_member);
- useIntConstPtr(&target.int_member);
+ useIntPtr(/*const*/target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+
+ (void)(&/*const*/target)->int_member;
+ useIntRef((&target)->int_member);
+
+ const S& const_target_ref = /*const*/target;
+ const S* const_target_ptr = &/*const*/target;
+ S* target_ptr = ⌖
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
- RunTest(R"(
+ RunTest<1>(R"(
void f(S* target) {
- useVal(*target);
- useConstRef(*target);
- useConstPtr(target);
+ useVal(*/*const*/target);
+ usePtr(target);
+ useConstRef(*/*const*/target);
+ useConstPtr(/*const*/target);
useConstPtrConstRef(/*const*/target);
+ usePtrConstPtr(&target);
/*const*/target->constMethod();
target->nonConstMethod();
- (*target)(ConstTag{});
+ (*/*const*/target)(ConstTag{});
(*target)[42];
target->operator[](42);
- useConstRef((*target));
+ useConstRef((*/*const*/target));
(/*const*/target)->constMethod();
- (void)(*target == *target);
- (void)*target;
- (void)target;
- S copy1 = *target;
- S copy2(*target);
- useInt(target->int_member);
- useIntConstRef(target->int_member);
- useIntPtr(target->ptr_member);
- useIntConstPtr(&target->int_member);
+ (void)(*/*const*/target == */*const*/target);
+ (void)*/*const*/target;
+ (void)/*const*/target;
+ /*const*/target;
+ S copy1 = */*const*/target;
+ S copy2(*/*const*/target);
+ /*const*/target->int_member;
+ useInt(/*const*/target->int_member);
+ useIntConstRef(/*const*/target->int_member);
+ useIntPtr(/*const*/target->ptr_member);
+ useIntConstPtr(&/*const*/target->int_member);
+
+ 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`.
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, ConstPtrVar) {
- RunTest(R"(
+ RunTest<1>(R"(
void f(const S* target) {
- useVal(*target);
- useConstRef(*target);
- useConstPtr(target);
- useConstPtrRef(target);
- useConstPtrPtr(&target);
- useConstPtrConstPtr(&target);
+ useVal(*/*const*/target);
+ useConstRef(*/*const*/target);
+ useConstPtr(/*const*/target);
+ useConstPtrRef(/*const*/target);
+ useConstPtrPtr(&/*const*/target);
+ useConstPtrConstPtr(&/*const*/target);
useConstPtrConstRef(/*const*/target);
/*const*/target->constMethod();
- (*target)(ConstTag{});
- (*target)[42];
+ (*/*const*/target)(ConstTag{});
+ (*/*const*/target)[42];
/*const*/target->operator[](42);
- (void)(*target == *target);
- (void)target;
- (void)*target;
- if(target) {}
- S copy1 = *target;
- S copy2(*target);
- useInt(target->int_member);
- useIntConstRef(target->int_member);
- useIntPtr(target->ptr_member);
- useIntConstPtr(&target->int_member);
+ (void)(*/*const*/target == */*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ /*const*/target;
+ if(/*const*/target) {}
+ S copy1 = */*const*/target;
+ S copy2(*/*const*/target);
+ /*const*/target->int_member;
+ useInt(/*const*/target->int_member);
+ useIntConstRef(/*const*/target->int_member);
+ useIntPtr(/*const*/target->ptr_member);
+ useIntConstPtr(&/*const*/target->int_member);
+
+ const S& const_target_ref = */*const*/target;
+ const S* const_target_ptr = /*const*/target;
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, ConstPtrPtrVar) {
- RunTest(R"(
+ RunTest<2>(R"(
void f(const S** target) {
- useVal(**target);
- useConstRef(**target);
- useConstPtr(*target);
- useConstPtrRef(*target);
- useConstPtrPtr(target);
- useConstPtrConstPtr(target);
- useConstPtrConstRef(*target);
- (void)target;
- (void)*target;
- (void)**target;
- if(target) {}
- if(*target) {}
- S copy1 = **target;
- S copy2(**target);
- useInt((*target)->int_member);
- useIntConstRef((*target)->int_member);
- useIntPtr((*target)->ptr_member);
- useIntConstPtr(&(*target)->int_member);
+ useVal(**/*const*/target);
+ useConstRef(**/*const*/target);
+ useConstPtr(*/*const*/target);
+ useConstPtrRef(*/*const*/target);
+ useConstPtrPtr(/*const*/target);
+ useConstPtrConstPtr(/*const*/target);
+ useConstPtrConstRef(*/*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ (void)**/*const*/target;
+ /*const*/target;
+ if(/*const*/target) {}
+ if(*/*const*/target) {}
+ S copy1 = **/*const*/target;
+ S copy2(**/*const*/target);
+ (*/*const*/target)->int_member;
+ useInt((*/*const*/target)->int_member);
+ useIntConstRef((*/*const*/target)->int_member);
+ useIntPtr((*/*const*/target)->ptr_member);
+ useIntConstPtr(&(*/*const*/target)->int_member);
+
+ const S& const_target_ref = **/*const*/target;
+ const S* const_target_ptr = */*const*/target;
}
)");
}
TEST(ConstReferenceDeclRefExprsTest, ConstPtrConstPtrVar) {
- RunTest(R"(
+ RunTest<2>(R"(
void f(const S* const* target) {
- useVal(**target);
- useConstRef(**target);
- useConstPtr(*target);
- useConstPtrConstPtr(target);
- useConstPtrConstRef(*target);
- (void)target;
- (void)target;
- (void)**target;
- if(target) {}
- if(*target) {}
- S copy1 = **target;
- S copy2(**target);
- useInt((*target)->int_member);
- useIntConstRef((*target)->int_member);
- useIntPtr((*target)->ptr_member);
- useIntConstPtr(&(*target)->int_member);
+ useVal(**/*const*/target);
+ useConstRef(**/*const*/target);
+ useConstPtr(*/*const*/target);
+ useConstPtrConstPtr(/*const*/target);
+ useConstPtrConstRef(*/*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ (void)**/*const*/target;
+ /*const*/target;
+ if(/*const*/target) {}
+ if(*/*const*/target) {}
+ S copy1 = **/*const*/target;
+ S copy2(**/*const*/target);
+ (*/*const*/target)->int_member;
+ useInt((*/*const*/target)->int_member);
+ useIntConstRef((*/*const*/target)->int_member);
+ useIntPtr((*/*const*/target)->ptr_member);
+ useIntConstPtr(&(*/*const*/target)->int_member);
+
+ const S& const_target_ref = **/*const*/target;
+ const S* const_target_ptr = */*const*/target;
}
)");
}
>From 019abc26cebac6f04bc1fbc068307345f6abaa27 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Mon, 26 Feb 2024 08:36:21 +0000
Subject: [PATCH 2/3] Spell types explicitly (no auto).
---
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 663691c519b8e9..3b14bb542d7d2c 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -77,7 +77,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
llvm::SmallVector<StackEntry, 4> Stack;
Stack.emplace_back(&Node, Indirections);
- auto &Ctx = Finder->getASTContext();
+ AstContext &Ctx = Finder->getASTContext();
while (!Stack.empty()) {
const StackEntry Entry = Stack.back();
@@ -96,7 +96,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
// Otherwise we have to look at the parents to see how the expression is
// used.
- const auto Parents = Ctx.getParents(*Entry.E);
+ const DynTypedNodeList Parents = Ctx.getParents(*Entry.E);
// Note: most nodes have a single parents, but there exist nodes that have
// several parents, such as `InitListExpr` that have semantic and syntactic
// forms.
>From cd2052a708365117237d36b86483d37465f6517a Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Mon, 26 Feb 2024 08:37:58 +0000
Subject: [PATCH 3/3] remove braces in single-line conditions
---
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 3b14bb542d7d2c..88b7f9d9f46785 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -90,9 +90,8 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
assert(Ty->isPointerType());
Ty = Ty->getPointeeType().getCanonicalType();
}
- if (Ty.isConstQualified()) {
+ if (Ty.isConstQualified())
continue;
- }
// Otherwise we have to look at the parents to see how the expression is
// used.
@@ -143,9 +142,8 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
continue;
case CK_LValueToRValue: {
// An rvalue is immutable.
- if (Entry.Indirections == 0) {
+ if (Entry.Indirections == 0)
continue;
- }
Stack.emplace_back(Cast, Entry.Indirections);
continue;
}
More information about the cfe-commits
mailing list