Skip to content

Commit 6d49d56

Browse files
nicovanknicovank
committed
[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.
1 parent 86e5c54 commit 6d49d56

File tree

5 files changed

+256
-43
lines changed

5 files changed

+256
-43
lines changed

‎clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp

Lines changed: 64 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,29 +14,81 @@ using namespace clang::ast_matchers;
1414

1515
namespace clang::tidy::readability {
1616

17+
namespace {
18+
struct NotMatchingBoundType {
19+
NotMatchingBoundType(std::string ArgumentTypeBoundID, DynTypedNode Node)
20+
: ArgumentTypeBoundID(std::move(ArgumentTypeBoundID)),
21+
Node(std::move(Node)) {}
22+
bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
23+
const Type *ParamType = Node.get<Type>();
24+
const Type *ArgType = Nodes.getNodeAs<Type>(ArgumentTypeBoundID);
25+
if (!ParamType || !ArgType) {
26+
return true;
27+
}
28+
29+
ParamType = ParamType->getUnqualifiedDesugaredType();
30+
ArgType = ArgType->getUnqualifiedDesugaredType();
31+
32+
if (ParamType->isReferenceType()) {
33+
ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
34+
}
35+
36+
while (ParamType->isPointerType()) {
37+
if (!ArgType->isPointerType()) {
38+
return true;
39+
}
40+
41+
ParamType = ParamType->getPointeeType()->getUnqualifiedDesugaredType();
42+
ArgType = ArgType->getPointeeType()->getUnqualifiedDesugaredType();
43+
}
44+
45+
return ParamType != ArgType;
46+
}
47+
48+
private:
49+
std::string ArgumentTypeBoundID;
50+
DynTypedNode Node;
51+
};
52+
53+
AST_MATCHER_P(Type, matchesBoundType, std::string, ArgumentTypeBoundID) {
54+
return Builder->removeBindings(
55+
NotMatchingBoundType(ArgumentTypeBoundID, DynTypedNode::create(Node)));
56+
}
57+
} // namespace
58+
1759
void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
18-
const auto SupportedContainers = hasType(
19-
hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
20-
hasAnyName("::std::set", "::std::unordered_set", "::std::map",
21-
"::std::unordered_map", "::std::multiset",
22-
"::std::unordered_multiset", "::std::multimap",
23-
"::std::unordered_multimap"))))));
60+
const auto HasContainsMethod = hasMethod(cxxMethodDecl(
61+
isConst(), parameterCountIs(1), returns(booleanType()),
62+
hasName("contains"), unless(isDeleted()), isPublic(),
63+
hasParameter(0, hasType(matchesBoundType("argumentType")))));
64+
const auto ContainerWithContains = hasType(
65+
hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(anyOf(
66+
HasContainsMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
67+
cxxRecordDecl(HasContainsMethod)))))))))));
68+
69+
// Hack in CountCall and FindCall: hasArgument(0, ...) always ignores implicit
70+
// casts. We do not want this behavior. We use hasAnyArgument instead, which
71+
// does not ignore implicit casts. Thankfully, we only have one argument in
72+
// every case making this possible. Really, hasArgument should also respect
73+
// the current traversal mode. GitHub issues #54919 and #75754 track this.
2474

2575
const auto CountCall =
26-
cxxMemberCallExpr(on(SupportedContainers),
76+
cxxMemberCallExpr(argumentCountIs(1),
2777
callee(cxxMethodDecl(hasName("count"))),
28-
argumentCountIs(1))
78+
hasAnyArgument(hasType(type().bind("argumentType"))),
79+
on(ContainerWithContains))
2980
.bind("call");
3081

3182
const auto FindCall =
32-
cxxMemberCallExpr(on(SupportedContainers),
83+
cxxMemberCallExpr(argumentCountIs(1),
3384
callee(cxxMethodDecl(hasName("find"))),
34-
argumentCountIs(1))
85+
hasAnyArgument(hasType(type().bind("argumentType"))),
86+
on(ContainerWithContains))
3587
.bind("call");
3688

37-
const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
89+
const auto EndCall = cxxMemberCallExpr(argumentCountIs(0),
3890
callee(cxxMethodDecl(hasName("end"))),
39-
argumentCountIs(0));
91+
on(ContainerWithContains));
4092

4193
const auto Literal0 = integerLiteral(equals(0));
4294
const auto Literal1 = integerLiteral(equals(1));

‎clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@
1313

1414
namespace clang::tidy::readability {
1515

16-
/// Finds usages of `container.count()` and `find() == end()` which should be
17-
/// replaced by a call to the `container.contains()` method introduced in C++20.
16+
/// Finds usages of ``container.count()`` and
17+
/// ``container.find() == container.end()`` which should be replaced by a call
18+
/// to the ``container.contains()`` method.
1819
///
1920
/// For the user-facing documentation see:
2021
/// http://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html
@@ -24,10 +25,11 @@ class ContainerContainsCheck : public ClangTidyCheck {
2425
: ClangTidyCheck(Name, Context) {}
2526
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
2627
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
27-
28-
protected:
2928
bool isLanguageVersionSupported(const LangOptions &LO) const final {
30-
return LO.CPlusPlus20;
29+
return LO.CPlusPlus;
30+
}
31+
std::optional<TraversalKind> getCheckTraversalKind() const override {
32+
return TK_AsIs;
3133
}
3234
};
3335

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,11 @@ Changes in existing checks
123123
<clang-tidy/checks/modernize/use-std-print>` check to support replacing
124124
member function calls too.
125125

126-
- Improved :doc:`readablility-implicit-bool-conversion
126+
- Improved :doc:`readability-container-contains
127+
<clang-tidy/checks/readability/container-contains>` check
128+
to let it work on any class that has a ``contains`` method.
129+
130+
- Improved :doc:`readability-implicit-bool-conversion
127131
<clang-tidy/checks/readability/implicit-bool-conversion>` check
128132
by adding the option `UseUpperCaseLiteralSuffix` to select the
129133
case of the literal suffix in fixes.

‎clang-tools-extra/docs/clang-tidy/checks/readability/container-contains.rst

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,31 @@
33
readability-container-contains
44
==============================
55

6-
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.
6+
Finds usages of ``container.count()`` and
7+
``container.find() == container.end()`` which should be replaced by a call to
8+
the ``container.contains()`` method.
79

8-
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.
10+
Whether an element is contained inside a container should be checked with
11+
``contains`` instead of ``count``/``find`` because ``contains`` conveys the
12+
intent more clearly. Furthermore, for containers which permit multiple entries
13+
per key (``multimap``, ``multiset``, ...), ``contains`` is more efficient than
14+
``count`` because ``count`` has to do unnecessary additional work.
915

1016
Examples:
1117

12-
=========================================== ==============================
13-
Initial expression Result
14-
------------------------------------------- ------------------------------
15-
``myMap.find(x) == myMap.end()`` ``!myMap.contains(x)``
16-
``myMap.find(x) != myMap.end()`` ``myMap.contains(x)``
17-
``if (myMap.count(x))`` ``if (myMap.contains(x))``
18-
``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)``
19-
``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)``
20-
``bool exists = myMap.count(x) >= 1`` ``bool exists = myMap.contains(x)``
21-
``bool missing = myMap.count(x) == 0`` ``bool missing = !myMap.contains(x)``
22-
=========================================== ==============================
18+
========================================= ==============================
19+
Initial expression Result
20+
----------------------------------------- ------------------------------
21+
``myMap.find(x) == myMap.end()`` ``!myMap.contains(x)``
22+
``myMap.find(x) != myMap.end()`` ``myMap.contains(x)``
23+
``if (myMap.count(x))`` ``if (myMap.contains(x))``
24+
``bool exists = myMap.count(x)`` ``bool exists = myMap.contains(x)``
25+
``bool exists = myMap.count(x) > 0`` ``bool exists = myMap.contains(x)``
26+
``bool exists = myMap.count(x) >= 1`` ``bool exists = myMap.contains(x)``
27+
``bool missing = myMap.count(x) == 0`` ``bool missing = !myMap.contains(x)``
28+
========================================= ==============================
2329

24-
This check applies to ``std::set``, ``std::unordered_set``, ``std::map``, ``std::unordered_map`` and the corresponding multi-key variants.
25-
It is only active for C++20 and later, as the ``contains`` method was only added in C++20.
30+
This check will apply to any class that has a ``contains`` method, notably
31+
including ``std::set``, ``std::unordered_set``, ``std::map``, and
32+
``std::unordered_map`` as of C++20, and ``std::string`` and ``std::string_view``
33+
as of C++23.

‎clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

Lines changed: 157 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ int testMacroExpansion(std::unordered_set<int> &MySet) {
240240
return 0;
241241
}
242242

243-
// The following map has the same interface like `std::map`.
243+
// The following map has the same interface as `std::map`.
244244
template <class Key, class T>
245245
struct CustomMap {
246246
unsigned count(const Key &K) const;
@@ -249,13 +249,160 @@ struct CustomMap {
249249
void *end();
250250
};
251251

252-
// The clang-tidy check is currently hard-coded against the `std::` containers
253-
// and hence won't annotate the following instance. We might change this in the
254-
// future and also detect the following case.
255-
void *testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
256-
if (MyMap.count(0))
257-
// NO-WARNING.
258-
// CHECK-FIXES: if (MyMap.count(0))
259-
return nullptr;
260-
return MyMap.find(2);
252+
void testDifferentCheckTypes(CustomMap<int, int> &MyMap) {
253+
if (MyMap.count(0)) {};
254+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
255+
// CHECK-FIXES: if (MyMap.contains(0)) {};
256+
257+
MyMap.find(0) != MyMap.end();
258+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
259+
// CHECK-FIXES: MyMap.contains(0);
260+
}
261+
262+
struct MySubmap : public CustomMap<int, int> {};
263+
264+
void testSubclass(MySubmap& MyMap) {
265+
if (MyMap.count(0)) {};
266+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
267+
// CHECK-FIXES: if (MyMap.contains(0)) {};
268+
}
269+
270+
using UsingMap = CustomMap<int, int>;
271+
struct MySubmap2 : public UsingMap {};
272+
using UsingMap2 = MySubmap2;
273+
274+
void testUsing(UsingMap2& MyMap) {
275+
if (MyMap.count(0)) {};
276+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
277+
// CHECK-FIXES: if (MyMap.contains(0)) {};
278+
}
279+
280+
template <class Key, class T>
281+
struct CustomMapContainsDeleted {
282+
unsigned count(const Key &K) const;
283+
bool contains(const Key &K) const = delete;
284+
void *find(const Key &K);
285+
void *end();
286+
};
287+
288+
struct SubmapContainsDeleted : public CustomMapContainsDeleted<int, int> {};
289+
290+
void testContainsDeleted(CustomMapContainsDeleted<int, int> &MyMap,
291+
SubmapContainsDeleted &MyMap2) {
292+
// No warning if the `contains` method is deleted.
293+
if (MyMap.count(0)) {};
294+
if (MyMap2.count(0)) {};
295+
}
296+
297+
template <class Key, class T>
298+
struct CustomMapPrivateContains {
299+
unsigned count(const Key &K) const;
300+
void *find(const Key &K);
301+
void *end();
302+
303+
private:
304+
bool contains(const Key &K) const;
305+
};
306+
307+
struct SubmapPrivateContains : public CustomMapPrivateContains<int, int> {};
308+
309+
void testPrivateContains(CustomMapPrivateContains<int, int> &MyMap,
310+
SubmapPrivateContains &MyMap2) {
311+
// No warning if the `contains` method is not public.
312+
if (MyMap.count(0)) {};
313+
if (MyMap2.count(0)) {};
314+
}
315+
316+
struct MyString {};
317+
318+
struct WeirdNonMatchingContains {
319+
unsigned count(char) const;
320+
bool contains(const MyString&) const;
321+
};
322+
323+
void testWeirdNonMatchingContains(WeirdNonMatchingContains &MyMap) {
324+
// No warning if there is no `contains` method with the right type.
325+
if (MyMap.count('a')) {};
326+
}
327+
328+
template <class T>
329+
struct SmallPtrSet {
330+
using ConstPtrType = const T*;
331+
unsigned count(ConstPtrType Ptr) const;
332+
bool contains(ConstPtrType Ptr) const;
333+
};
334+
335+
template <class T>
336+
struct SmallPtrPtrSet {
337+
using ConstPtrType = const T**;
338+
unsigned count(ConstPtrType Ptr) const;
339+
bool contains(ConstPtrType Ptr) const;
340+
};
341+
342+
template <class T>
343+
struct SmallPtrPtrPtrSet {
344+
using ConstPtrType = const T***;
345+
unsigned count(ConstPtrType Ptr) const;
346+
bool contains(ConstPtrType Ptr) const;
347+
};
348+
349+
void testSmallPtrSet(const int ***Ptr,
350+
SmallPtrSet<int> &MySet,
351+
SmallPtrPtrSet<int> &MySet2,
352+
SmallPtrPtrPtrSet<int> &MySet3) {
353+
if (MySet.count(**Ptr)) {};
354+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
355+
// CHECK-FIXES: if (MySet.contains(**Ptr)) {};
356+
if (MySet2.count(*Ptr)) {};
357+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
358+
// CHECK-FIXES: if (MySet2.contains(*Ptr)) {};
359+
if (MySet3.count(Ptr)) {};
360+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
361+
// CHECK-FIXES: if (MySet3.contains(Ptr)) {};
362+
}
363+
364+
struct X {};
365+
struct Y : public X {};
366+
367+
void testSubclassEntry(SmallPtrSet<X>& Set, Y* Entry) {
368+
if (Set.count(Entry)) {}
369+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
370+
// CHECK-FIXES: if (Set.contains(Entry)) {}
371+
}
372+
373+
struct WeirdPointerApi {
374+
unsigned count(int** Ptr) const;
375+
bool contains(int* Ptr) const;
376+
};
377+
378+
void testWeirdApi(WeirdPointerApi& Set, int* E) {
379+
if (Set.count(&E)) {}
380+
}
381+
382+
void testIntUnsigned(std::set<int>& S, unsigned U) {
383+
if (S.count(U)) {}
384+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
385+
// CHECK-FIXES: if (S.contains(U)) {}
386+
}
387+
388+
template <class T>
389+
struct CustomSetConvertible {
390+
unsigned count(const T &K) const;
391+
bool contains(const T &K) const;
392+
};
393+
394+
struct A {};
395+
struct B { B() = default; B(const A&) {} };
396+
struct C { operator A() const; };
397+
398+
void testConvertibleTypes() {
399+
CustomSetConvertible<B> MyMap;
400+
if (MyMap.count(A())) {};
401+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
402+
// CHECK-FIXES: if (MyMap.contains(A())) {};
403+
404+
CustomSetConvertible<A> MyMap2;
405+
if (MyMap2.count(C())) {};
406+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
407+
// CHECK-FIXES: if (MyMap2.contains(C())) {};
261408
}

0 commit comments

Comments
 (0)