Skip to content

Commit 72db8b6

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 c96ee0f commit 72db8b6

File tree

5 files changed

+299
-93
lines changed

5 files changed

+299
-93
lines changed

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

Lines changed: 88 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -13,90 +13,115 @@
1313
using namespace clang::ast_matchers;
1414

1515
namespace clang::tidy::readability {
16-
1716
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"))))));
17+
const auto HasContainsMatchingArgType = hasMethod(
18+
cxxMethodDecl(isConst(), parameterCountIs(1), returns(booleanType()),
19+
hasName("contains"), unless(isDeleted()), isPublic(),
20+
hasParameter(0, hasType(hasUnqualifiedDesugaredType(
21+
equalsBoundNode("parameterType"))))));
2422

2523
const auto CountCall =
26-
cxxMemberCallExpr(on(SupportedContainers),
27-
callee(cxxMethodDecl(hasName("count"))),
28-
argumentCountIs(1))
24+
cxxMemberCallExpr(
25+
argumentCountIs(1),
26+
callee(cxxMethodDecl(
27+
hasName("count"),
28+
hasParameter(0, hasType(hasUnqualifiedDesugaredType(
29+
type().bind("parameterType")))),
30+
ofClass(cxxRecordDecl(HasContainsMatchingArgType)))))
2931
.bind("call");
3032

3133
const auto FindCall =
32-
cxxMemberCallExpr(on(SupportedContainers),
33-
callee(cxxMethodDecl(hasName("find"))),
34-
argumentCountIs(1))
34+
cxxMemberCallExpr(
35+
argumentCountIs(1),
36+
callee(cxxMethodDecl(
37+
hasName("find"),
38+
hasParameter(0, hasType(hasUnqualifiedDesugaredType(
39+
type().bind("parameterType")))),
40+
ofClass(cxxRecordDecl(HasContainsMatchingArgType)))))
3541
.bind("call");
3642

37-
const auto EndCall = cxxMemberCallExpr(on(SupportedContainers),
38-
callee(cxxMethodDecl(hasName("end"))),
39-
argumentCountIs(0));
43+
const auto EndCall = cxxMemberCallExpr(
44+
argumentCountIs(0),
45+
callee(
46+
cxxMethodDecl(hasName("end"),
47+
// In the matchers below, FindCall should always appear
48+
// before EndCall so 'parameterType' is properly bound.
49+
ofClass(cxxRecordDecl(HasContainsMatchingArgType)))));
4050

4151
const auto Literal0 = integerLiteral(equals(0));
4252
const auto Literal1 = integerLiteral(equals(1));
4353

44-
auto AddSimpleMatcher = [&](auto Matcher) {
45-
Finder->addMatcher(
46-
traverse(TK_IgnoreUnlessSpelledInSource, std::move(Matcher)), this);
47-
};
48-
4954
// Find membership tests which use `count()`.
5055
Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
5156
hasSourceExpression(CountCall))
5257
.bind("positiveComparison"),
5358
this);
54-
AddSimpleMatcher(
55-
binaryOperator(hasLHS(CountCall), hasOperatorName("!="), hasRHS(Literal0))
56-
.bind("positiveComparison"));
57-
AddSimpleMatcher(
58-
binaryOperator(hasLHS(Literal0), hasOperatorName("!="), hasRHS(CountCall))
59-
.bind("positiveComparison"));
60-
AddSimpleMatcher(
61-
binaryOperator(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
62-
.bind("positiveComparison"));
63-
AddSimpleMatcher(
64-
binaryOperator(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
65-
.bind("positiveComparison"));
66-
AddSimpleMatcher(
67-
binaryOperator(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1))
68-
.bind("positiveComparison"));
69-
AddSimpleMatcher(
70-
binaryOperator(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall))
71-
.bind("positiveComparison"));
59+
Finder->addMatcher(
60+
binaryOperator(hasOperatorName("!="),
61+
hasOperands(ignoringParenImpCasts(CountCall),
62+
ignoringParenImpCasts(Literal0)))
63+
.bind("positiveComparison"),
64+
this);
65+
Finder->addMatcher(binaryOperator(hasOperatorName(">"),
66+
hasLHS(ignoringParenImpCasts(CountCall)),
67+
hasRHS(ignoringParenImpCasts(Literal0)))
68+
.bind("positiveComparison"),
69+
this);
70+
Finder->addMatcher(binaryOperator(hasOperatorName("<"),
71+
hasLHS(ignoringParenImpCasts(Literal0)),
72+
hasRHS(ignoringParenImpCasts(CountCall)))
73+
.bind("positiveComparison"),
74+
this);
75+
Finder->addMatcher(binaryOperator(hasOperatorName(">="),
76+
hasLHS(ignoringParenImpCasts(CountCall)),
77+
hasRHS(ignoringParenImpCasts(Literal1)))
78+
.bind("positiveComparison"),
79+
this);
80+
Finder->addMatcher(binaryOperator(hasOperatorName("<="),
81+
hasLHS(ignoringParenImpCasts(Literal1)),
82+
hasRHS(ignoringParenImpCasts(CountCall)))
83+
.bind("positiveComparison"),
84+
this);
7285

7386
// Find inverted membership tests which use `count()`.
74-
AddSimpleMatcher(
75-
binaryOperator(hasLHS(CountCall), hasOperatorName("=="), hasRHS(Literal0))
76-
.bind("negativeComparison"));
77-
AddSimpleMatcher(
78-
binaryOperator(hasLHS(Literal0), hasOperatorName("=="), hasRHS(CountCall))
79-
.bind("negativeComparison"));
80-
AddSimpleMatcher(
81-
binaryOperator(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0))
82-
.bind("negativeComparison"));
83-
AddSimpleMatcher(
84-
binaryOperator(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall))
85-
.bind("negativeComparison"));
86-
AddSimpleMatcher(
87-
binaryOperator(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
88-
.bind("negativeComparison"));
89-
AddSimpleMatcher(
90-
binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
91-
.bind("negativeComparison"));
87+
Finder->addMatcher(
88+
binaryOperator(hasOperatorName("=="),
89+
hasOperands(ignoringParenImpCasts(CountCall),
90+
ignoringParenImpCasts(Literal0)))
91+
.bind("negativeComparison"),
92+
this);
93+
Finder->addMatcher(binaryOperator(hasOperatorName("<="),
94+
hasLHS(ignoringParenImpCasts(CountCall)),
95+
hasRHS(ignoringParenImpCasts(Literal0)))
96+
.bind("negativeComparison"),
97+
this);
98+
Finder->addMatcher(binaryOperator(hasOperatorName(">="),
99+
hasLHS(ignoringParenImpCasts(Literal0)),
100+
hasRHS(ignoringParenImpCasts(CountCall)))
101+
.bind("negativeComparison"),
102+
this);
103+
Finder->addMatcher(binaryOperator(hasOperatorName("<"),
104+
hasLHS(ignoringParenImpCasts(CountCall)),
105+
hasRHS(ignoringParenImpCasts(Literal1)))
106+
.bind("negativeComparison"),
107+
this);
108+
Finder->addMatcher(binaryOperator(hasOperatorName(">"),
109+
hasLHS(ignoringParenImpCasts(Literal1)),
110+
hasRHS(ignoringParenImpCasts(CountCall)))
111+
.bind("negativeComparison"),
112+
this);
92113

93114
// Find membership tests based on `find() == end()`.
94-
AddSimpleMatcher(
95-
binaryOperator(hasLHS(FindCall), hasOperatorName("!="), hasRHS(EndCall))
96-
.bind("positiveComparison"));
97-
AddSimpleMatcher(
98-
binaryOperator(hasLHS(FindCall), hasOperatorName("=="), hasRHS(EndCall))
99-
.bind("negativeComparison"));
115+
Finder->addMatcher(binaryOperator(hasOperatorName("!="),
116+
hasOperands(ignoringParenImpCasts(FindCall),
117+
ignoringParenImpCasts(EndCall)))
118+
.bind("positiveComparison"),
119+
this);
120+
Finder->addMatcher(binaryOperator(hasOperatorName("=="),
121+
hasOperands(ignoringParenImpCasts(FindCall),
122+
ignoringParenImpCasts(EndCall)))
123+
.bind("negativeComparison"),
124+
this);
100125
}
101126

102127
void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {

‎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: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,10 @@ Changes in existing checks
136136
<clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
137137
placeholder when lexer cannot get source text.
138138

139+
- Improved :doc:`readability-container-contains
140+
<clang-tidy/checks/readability/container-contains>` check
141+
to let it work on any class that has a ``contains`` method.
142+
139143
- Improved :doc:`readability-implicit-bool-conversion
140144
<clang-tidy/checks/readability/implicit-bool-conversion>` check
141145
by adding the option `UseUpperCaseLiteralSuffix` to select the

‎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.

0 commit comments

Comments
 (0)