[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