[clang-tools-extra] [clang-tidy][readability-container-contains] Extend to any class with contains (PR #107521)
Nicolas van Kempen via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 11 22:00:28 PDT 2024
https://github.com/nicovank updated https://github.com/llvm/llvm-project/pull/107521
>From 2033e5b44787b89244ccd76ac9422162bcfe8680 Mon Sep 17 00:00:00 2001
From: Nicolas van Kempen <nvankemp at gmail.com>
Date: Thu, 12 Sep 2024 00:54:39 -0400
Subject: [PATCH] [clang-tidy][readability-container-contains] Extend to any
class with contains
This check will now work out of the box with other containers that have a
`contains` method, such as `folly::F14` or Abseil containers.
It will also work with strings, which are basically just weird containers.
`std::string` and `std::string_view` will have a `contains` method starting with
C++23. `llvm::StringRef` and `folly::StringPiece` are examples of existing
implementations with a `contains` method.
---
.../readability/ContainerContainsCheck.cpp | 76 ++++++--
.../readability/ContainerContainsCheck.h | 12 +-
clang-tools-extra/docs/ReleaseNotes.rst | 6 +-
.../checks/readability/container-contains.rst | 38 ++--
.../readability/container-contains.cpp | 167 ++++++++++++++++--
5 files changed, 256 insertions(+), 43 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index dbb50a060e5960..2954e7e0e287d1 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -14,29 +14,81 @@ using namespace clang::ast_matchers;
namespace clang::tidy::readability {
+namespace {
+struct NotMatchingBoundType {
+ NotMatchingBoundType(std::string ArgumentTypeBoundID, DynTypedNode Node)
+ : ArgumentTypeBoundID(std::move(ArgumentTypeBoundID)),
+ Node(std::move(Node)) {}
+ bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+ const Type *ParamType = Node.get<Type>();
+ const Type *ArgType = Nodes.getNodeAs<Type>(ArgumentTypeBoundID);
+ if (!ParamType || !ArgType) {
+ return true;
+ }
+
+ ParamType = ParamType->getUnqualifiedDesugaredType();
+ ArgType = ArgType->getUnqualifiedDesugaredType();
+
+ if (ParamType->isReferenceType()) {
+ ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
+ }
+
+ while (ParamType->isPointerType()) {
+ if (!ArgType->isPointerType()) {
+ return true;
+ }
+
+ ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
+ ArgType = ArgType->getPointeeType()->getUnqualifiedDesugaredType();
+ }
+
+ return ParamType != ArgType;
+ }
+
+private:
+ std::string ArgumentTypeBoundID;
+ DynTypedNode Node;
+};
+
+AST_MATCHER_P(Type, matchesBoundType, std::string, ArgumentTypeBoundID) {
+ return Builder->removeBindings(
+ NotMatchingBoundType(ArgumentTypeBoundID, DynTypedNode::create(Node)));
+}
+} // namespace
+
void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
- const auto SupportedContainers = hasType(
- hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
- hasAnyName("::std::set", "::std::unordered_set", "::std::map",
- "::std::unordered_map", "::std::multiset",
- "::std::unordered_multiset", "::std::multimap",
- "::std::unordered_multimap"))))));
+ const auto HasContainsMethod = hasMethod(cxxMethodDecl(
+ isConst(), parameterCountIs(1), returns(booleanType()),
+ hasName("contains"), unless(isDeleted()), isPublic(),
+ hasParameter(0, hasType(matchesBoundType("argumentType")))));
+ const auto ContainerWithContains = hasType(
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(anyOf(
+ HasContainsMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+ cxxRecordDecl(HasContainsMethod)))))))))));
+
+ // Hack in CountCall and FindCall: hasArgument(0, ...) always ignores implicit
+ // casts. We do not want this behavior. We use hasAnyArgument instead, which
+ // does not ignore implicit casts. Thankfully, we only have one argument in
+ // every case making this possible. Really, hasArgument should also respect
+ // the current traversal mode. GitHub issues #54919 and #75754 track this.
const auto CountCall =
- cxxMemberCallExpr(on(SupportedContainers),
+ cxxMemberCallExpr(argumentCountIs(1),
callee(cxxMethodDecl(hasName("count"))),
- argumentCountIs(1))
+ hasAnyArgument(hasType(type().bind("argumentType"))),
+ on(ContainerWithContains))
.bind("call");
const auto FindCall =
- cxxMemberCallExpr(on(SupportedContainers),
+ cxxMemberCallExpr(argumentCountIs(1),
callee(cxxMethodDecl(hasName("find"))),
- argumentCountIs(1))
+ hasAnyArgument(hasType(type().bind("argumentType"))),
+ on(ContainerWithContains))
.bind("call");
- const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
+ const auto EndCall = cxxMemberCallExpr(argumentCountIs(0),
callee(cxxMethodDecl(hasName("end"))),
- argumentCountIs(0));
+ on(ContainerWithContains));
const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
index 2e8276d684cd79..27c2743b5358a0 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -13,8 +13,9 @@
namespace clang::tidy::readability {
-/// Finds usages of `container.count()` and `find() == end()` which should be
-/// replaced by a call to the `container.contains()` method introduced in C++20.
+/// Finds usages of ``container.count()`` and
+/// ``container.find() == container.end()`` which should be replaced by a call
+/// to the ``container.contains()`` method.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html
@@ -24,10 +25,11 @@ class ContainerContainsCheck : public ClangTidyCheck {
: ClangTidyCheck(Name, Context) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
-
-protected:
bool isLanguageVersionSupported(const LangOptions &LO) const final {
- return LO.CPlusPlus20;
+ return LO.CPlusPlus;
+ }
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_AsIs;
}
};
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1ad8cedc902cb5..8c1f1023829865 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -123,7 +123,11 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-std-print>` check to support replacing
member function calls too.
-- Improved :doc:`readablility-implicit-bool-conversion
+- Improved :doc:`readability-container-contains
+ <clang-tidy/checks/readability/container-contains>` check
+ to let it work on any class that has a ``contains`` method.
+
+- Improved :doc:`readability-implicit-bool-conversion
<clang-tidy/checks/readability/implicit-bool-conversion>` check
by adding the option `UseUpperCaseLiteralSuffix` to select the
case of the literal suffix in fixes.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst
index b28daecf7a2cf3..86aac019359ff9 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst
@@ -3,23 +3,31 @@
readability-container-contains
==============================
-Finds usages of ``container.count()`` and ``container.find() == container.end()`` which should be replaced by a call to the ``container.contains()`` method introduced in C++20.
+Finds usages of ``container.count()`` and
+``container.find() == container.end()`` which should be replaced by a call to
+the ``container.contains()`` method.
-Whether an element is contained inside a container should be checked with ``contains`` instead of ``count``/``find`` because ``contains`` conveys the intent more clearly. Furthermore, for containers which permit multiple entries per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than ``count`` because ``count`` has to do unnecessary additional work.
+Whether an element is contained inside a container should be checked with
+``contains`` instead of ``count``/``find`` because ``contains`` conveys the
+intent more clearly. Furthermore, for containers which permit multiple entries
+per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than
+``count`` because ``count`` has to do unnecessary additional work.
Examples:
-=========================================== ==============================
-Initial expression Result
-------------------------------------------- ------------------------------
-``myMap.find(x) == myMap.end()`` ``!myMap.contains(x)``
-``myMap.find(x) != myMap.end()`` ``myMap.contains(x)``
-``if (myMap.count(x))`` ``if (myMap.contains(x))``
-``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)``
-``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)``
-``bool exists = myMap.count(x) >= 1`` ``bool exists = myMap.contains(x)``
-``bool missing = myMap.count(x) == 0`` ``bool missing = !myMap.contains(x)``
-=========================================== ==============================
+========================================= ==============================
+Initial expression Result
+----------------------------------------- ------------------------------
+``myMap.find(x) == myMap.end()`` ``!myMap.contains(x)``
+``myMap.find(x) != myMap.end()`` ``myMap.contains(x)``
+``if (myMap.count(x))`` ``if (myMap.contains(x))``
+``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)``
+``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)``
+``bool exists = myMap.count(x) >= 1`` ``bool exists = myMap.contains(x)``
+``bool missing = myMap.count(x) == 0`` ``bool missing = !myMap.contains(x)``
+========================================= ==============================
-This check applies to ``std::set``, ``std::unordered_set``, ``std::map``, ``std::unordered_map`` and the corresponding multi-key variants.
-It is only active for C++20 and later, as the ``contains`` method was only added in C++20.
+This check will apply to any class that has a ``contains`` method, notably
+including ``std::set``, ``std::unordered_set``, ``std::map``, and
+``std::unordered_map`` as of C++20, and ``std::string`` and ``std::string_view``
+as of C++23.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
index 0ecb61b2e7df06..48b6e67c2091f7 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
@@ -240,7 +240,7 @@ int testMacroExpansion(std::unordered_set<int> &MySet) {
return 0;
}
-// The following map has the same interface like `std::map`.
+// The following map has the same interface as `std::map`.
template <class Key, class T>
struct CustomMap {
unsigned count(const Key &K) const;
@@ -249,13 +249,160 @@ struct CustomMap {
void *end();
};
-// The clang-tidy check is currently hard-coded against the `std::` containers
-// and hence won't annotate the following instance. We might change this in the
-// future and also detect the following case.
-void *testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
- if (MyMap.count(0))
- // NO-WARNING.
- // CHECK-FIXES: if (MyMap.count(0))
- return nullptr;
- return MyMap.find(2);
+void testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
+ if (MyMap.count(0)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MyMap.contains(0)) {};
+
+ MyMap.find(0) != MyMap.end();
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: MyMap.contains(0);
+}
+
+struct MySubmap : public CustomMap<int, int> {};
+
+void testSubclass(MySubmap& MyMap) {
+ if (MyMap.count(0)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MyMap.contains(0)) {};
+}
+
+using UsingMap = CustomMap<int, int>;
+struct MySubmap2 : public UsingMap {};
+using UsingMap2 = MySubmap2;
+
+void testUsing(UsingMap2& MyMap) {
+ if (MyMap.count(0)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MyMap.contains(0)) {};
+}
+
+template <class Key, class T>
+struct CustomMapContainsDeleted {
+ unsigned count(const Key &K) const;
+ bool contains(const Key &K) const = delete;
+ void *find(const Key &K);
+ void *end();
+};
+
+struct SubmapContainsDeleted : public CustomMapContainsDeleted<int, int> {};
+
+void testContainsDeleted(CustomMapContainsDeleted<int, int> &MyMap,
+ SubmapContainsDeleted &MyMap2) {
+ // No warning if the `contains` method is deleted.
+ if (MyMap.count(0)) {};
+ if (MyMap2.count(0)) {};
+}
+
+template <class Key, class T>
+struct CustomMapPrivateContains {
+ unsigned count(const Key &K) const;
+ void *find(const Key &K);
+ void *end();
+
+private:
+ bool contains(const Key &K) const;
+};
+
+struct SubmapPrivateContains : public CustomMapPrivateContains<int, int> {};
+
+void testPrivateContains(CustomMapPrivateContains<int, int> &MyMap,
+ SubmapPrivateContains &MyMap2) {
+ // No warning if the `contains` method is not public.
+ if (MyMap.count(0)) {};
+ if (MyMap2.count(0)) {};
+}
+
+struct MyString {};
+
+struct WeirdNonMatchingContains {
+ unsigned count(char) const;
+ bool contains(const MyString&) const;
+};
+
+void testWeirdNonMatchingContains(WeirdNonMatchingContains &MyMap) {
+ // No warning if there is no `contains` method with the right type.
+ if (MyMap.count('a')) {};
+}
+
+template <class T>
+struct SmallPtrSet {
+ using ConstPtrType = const T*;
+ unsigned count(ConstPtrType Ptr) const;
+ bool contains(ConstPtrType Ptr) const;
+};
+
+template <class T>
+struct SmallPtrPtrSet {
+ using ConstPtrType = const T**;
+ unsigned count(ConstPtrType Ptr) const;
+ bool contains(ConstPtrType Ptr) const;
+};
+
+template <class T>
+struct SmallPtrPtrPtrSet {
+ using ConstPtrType = const T***;
+ unsigned count(ConstPtrType Ptr) const;
+ bool contains(ConstPtrType Ptr) const;
+};
+
+void testSmallPtrSet(const int ***Ptr,
+ SmallPtrSet<int> &MySet,
+ SmallPtrPtrSet<int> &MySet2,
+ SmallPtrPtrPtrSet<int> &MySet3) {
+ if (MySet.count(**Ptr)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MySet.contains(**Ptr)) {};
+ if (MySet2.count(*Ptr)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MySet2.contains(*Ptr)) {};
+ if (MySet3.count(Ptr)) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MySet3.contains(Ptr)) {};
+}
+
+struct X {};
+struct Y : public X {};
+
+void testSubclassEntry(SmallPtrSet<X>& Set, Y* Entry) {
+ if (Set.count(Entry)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (Set.contains(Entry)) {}
+}
+
+struct WeirdPointerApi {
+ unsigned count(int** Ptr) const;
+ bool contains(int* Ptr) const;
+};
+
+void testWeirdApi(WeirdPointerApi& Set, int* E) {
+ if (Set.count(&E)) {}
+}
+
+void testIntUnsigned(std::set<int>& S, unsigned U) {
+ if (S.count(U)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (s.contains(u)) {}
+}
+
+template <class T>
+struct CustomSetConvertible {
+ unsigned count(const T &K) const;
+ bool contains(const T &K) const;
+};
+
+struct A {};
+struct B { B() = default; B(const A&) {} };
+struct C { operator A() const; };
+
+void testConvertibleTypes() {
+ CustomSetConvertible<B> MyMap;
+ if (MyMap.count(A())) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MyMap.contains(A())) {};
+
+ CustomSetConvertible<A> MyMap2;
+ if (MyMap2.count(C())) {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: if (MyMap2.contains(C())) {};
}
More information about the cfe-commits
mailing list