[clang-tools-extra] [clang-tidy] `doesNotMutateObject`: Handle calls to member functions … (PR #94362)

Clement Courbet via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 6 02:12:19 PDT 2024


https://github.com/legrosbuffle updated https://github.com/llvm/llvm-project/pull/94362

>From 8a7e3ee49295b55193440da6b796c9ada43ee5ef Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Tue, 4 Jun 2024 12:49:39 +0000
Subject: [PATCH 1/2] [clang-tidy] `doesNotMutateObject`: Handle calls to
 member functions 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];
}
```
---
 .../UnnecessaryCopyInitialization.cpp         |  21 ++-
 .../clang-tidy/utils/DeclRefExprUtils.cpp     | 163 +++++++++++++++++-
 clang-tools-extra/docs/ReleaseNotes.rst       |   4 +-
 .../unnecessary-copy-initialization.cpp       |  29 +++-
 .../clang-tidy/DeclRefExprUtilsTest.cpp       |  39 ++++-
 5 files changed, 229 insertions(+), 27 deletions(-)

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);
     }
 )");
 }

>From 6c521ec94ec01ebff737c8e732377b44eae53be3 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Thu, 6 Jun 2024 08:05:56 +0000
Subject: [PATCH 2/2] address comments, add tests for overloads with distinct
 types

---
 .../clang-tidy/utils/DeclRefExprUtils.cpp     | 73 ++++++++++---------
 .../clang-tidy/DeclRefExprUtilsTest.cpp       | 13 +++-
 2 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 4ee8a3628061b..7cab214a41b45 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -36,37 +36,49 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
     Nodes.insert(Match.getNodeAs<Node>(ID));
 }
 
+// Returns true if both types refer 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 `D` and `O` have the same parameter types.
+bool hasSameParameterTypes(const CXXMethodDecl &D, const CXXMethodDecl &O) {
+  if (D.getNumParams() != O.getNumParams())
+    return false;
+  for (int I = 0, E = D.getNumParams(); I < E; ++I) {
+    if (!isSameTypeIgnoringConst(D.getParamDecl(I)->getType(),
+                                 O.getParamDecl(I)->getType()))
+      return false;
+  }
+  return true;
+}
+
 // If `D` has a const-qualified overload with otherwise identical
-// ref-qualifiers, returns that overload.
+// ref-qualifiers and parameter types, returns that overload.
 const CXXMethodDecl *findConstOverload(const CXXMethodDecl &D) {
   assert(!D.isConst());
 
-  DeclContext::lookup_result lookup_result =
+  DeclContext::lookup_result LookupResult =
       D.getParent()->lookup(D.getNameInfo().getName());
-  if (lookup_result.isSingleResult()) {
+  if (LookupResult.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;
-    }
+  for (const Decl *Overload : LookupResult) {
+    const CXXMethodDecl *O = dyn_cast<CXXMethodDecl>(Overload);
+    if (O && !O->isDeleted() && O->isConst() &&
+        O->getRefQualifier() == D.getRefQualifier() &&
+        hasSameParameterTypes(D, *O))
+      return O;
   }
   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) {
@@ -124,21 +136,14 @@ bool isLikelyShallowConst(const CXXMethodDecl &M) {
   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 OverloadTy->isReferenceType() &&
+           pointsToSameTypeIgnoringConst(CallTy, OverloadTy);
   }
-  return true;
+  if (CallTy->isPointerType()) {
+    return OverloadTy->isPointerType() &&
+           pointsToSameTypeIgnoringConst(CallTy, OverloadTy);
+  }
+  return isSameTypeIgnoringConst(CallTy, OverloadTy);
 }
 
 // A matcher that matches DeclRefExprs that are used in ways such that the
@@ -268,7 +273,7 @@ AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
             // the method's return value (C).
             const auto MemberParents = Ctx.getParents(*Member);
             assert(MemberParents.size() == 1);
-            const CallExpr *Call = MemberParents[0].get<CallExpr>();
+            const auto *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");
diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
index af314d125d414..064e04c932de8 100644
--- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -46,6 +46,7 @@ template <int Indirections> void RunTest(StringRef Snippet) {
   StringRef CommonCode = R"(
     struct ConstTag{};
     struct NonConstTag{};
+    struct Tag1{};
 
     struct S {
       void constMethod() const;
@@ -61,6 +62,10 @@ template <int Indirections> void RunTest(StringRef Snippet) {
 
       int& at(int);
       const int& at(int) const;
+      const int& at(Tag1);
+
+      int& weird_overload();
+      const double& weird_overload() const;
 
       bool operator==(const S&) const;
 
@@ -194,7 +199,7 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
       /*const*/target(ConstTag{});
       /*const*/target[42];
       /*const*/target(ConstTag{});
-      /*const*/target(NonConstTag{});
+      target(NonConstTag{});
       useRef(target);
       usePtr(&target);
       useConstRef((/*const*/target));
@@ -222,6 +227,8 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
       const int civ = /*const*/target.at(3);
       const int& cir = /*const*/target.at(3);
       int& ir = target.at(3);
+      target.at(Tag1{});
+      target.weird_overload();
     }
 )");
 }
@@ -266,6 +273,8 @@ TEST(ConstReferenceDeclRefExprsTest, RefVar) {
       const int civ = /*const*/target.at(3);
       const int& cir = /*const*/target.at(3);
       int& ir = target.at(3);
+      target.at(Tag1{});
+      target.weird_overload();
     }
 )");
 }
@@ -308,6 +317,8 @@ TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
       const int civ = /*const*/target->at(3);
       const int& cir = /*const*/target->at(3);
       int& ir = target->at(3);
+      target->at(Tag1{});
+      target->weird_overload();
     }
 )");
 }



More information about the cfe-commits mailing list