Skip to content

[clang-tidy][readability-container-contains] Fix matching of non-binaryOperator cases #110386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 23, 2024
Merged

[clang-tidy][readability-container-contains] Fix matching of non-binaryOperator cases #110386

merged 1 commit into from
Oct 23, 2024

Conversation

nicovank
Copy link
Contributor

Fix #79437.

It works with non-mock std::map:

# All cases detailed in #79437.
> cat tmp.cpp
#include <map>
bool a(std::map<int, int> &m, int key) { return m.find(key) != m.end(); }
bool b(std::map<int, int> &m, int key) { return m.count(key) > 0; }
bool c(std::map<int, int> &m, int key) { return m.find(key) == m.end(); }
bool d(std::map<int, int> &m, int key) { return m.count(key) == 0; }
bool e(std::map<int, int> *m, int key) { return m->find(key) != m->end(); }
bool f(std::map<int, int> *m, int key) { return m->find(key) == m->end(); }
bool g(std::map<int, int> *m, int key) { return m->count(key) > 0; }
bool h(std::map<int, int> *m, int key) { return m->count(key) == 0; }

> ./build/bin/clang-tidy -checks="-*,readability-container-contains" tmp.cpp -- -std=c++20
8 warnings generated.
tmp.cpp:2:51: warning: use 'contains' to check for membership [readability-container-contains]
    2 | bool a(std::map<int, int> &m, int key) { return m.find(key) != m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                   contains
tmp.cpp:3:51: warning: use 'contains' to check for membership [readability-container-contains]
    3 | bool b(std::map<int, int> &m, int key) { return m.count(key) > 0; }
      |                                                   ^~~~~      ~~~
      |                                                   contains
tmp.cpp:4:51: warning: use 'contains' to check for membership [readability-container-contains]
    4 | bool c(std::map<int, int> &m, int key) { return m.find(key) == m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                 ! contains
tmp.cpp:5:51: warning: use 'contains' to check for membership [readability-container-contains]
    5 | bool d(std::map<int, int> &m, int key) { return m.count(key) == 0; }
      |                                                   ^~~~~      ~~~~
      |                                                 ! contains
tmp.cpp:6:52: warning: use 'contains' to check for membership [readability-container-contains]
    6 | bool e(std::map<int, int> *m, int key) { return m->find(key) != m->end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                    contains
tmp.cpp:7:52: warning: use 'contains' to check for membership [readability-container-contains]
    7 | bool f(std::map<int, int> *m, int key) { return m->find(key) == m->end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                 !  contains
tmp.cpp:8:52: warning: use 'contains' to check for membership [readability-container-contains]
    8 | bool g(std::map<int, int> *m, int key) { return m->count(key) > 0; }
      |                                                    ^~~~~      ~~~
      |                                                    contains
tmp.cpp:9:52: warning: use 'contains' to check for membership [readability-container-contains]
    9 | bool h(std::map<int, int> *m, int key) { return m->count(key) == 0; }
      |                                                    ^~~~~      ~~~~
      |                                                 !  contains

@llvmbot
Copy link
Member

llvmbot commented Sep 28, 2024 •
edited
Loading

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Nicolas van Kempen (nicovank)

Changes

Fix #79437.

It works with non-mock std::map:

# All cases detailed in #<!-- -->79437.
&gt; cat tmp.cpp
#include &lt;map&gt;
bool a(std::map&lt;int, int&gt; &amp;m, int key) { return m.find(key) != m.end(); }
bool b(std::map&lt;int, int&gt; &amp;m, int key) { return m.count(key) &gt; 0; }
bool c(std::map&lt;int, int&gt; &amp;m, int key) { return m.find(key) == m.end(); }
bool d(std::map&lt;int, int&gt; &amp;m, int key) { return m.count(key) == 0; }
bool e(std::map&lt;int, int&gt; *m, int key) { return m-&gt;find(key) != m-&gt;end(); }
bool f(std::map&lt;int, int&gt; *m, int key) { return m-&gt;find(key) == m-&gt;end(); }
bool g(std::map&lt;int, int&gt; *m, int key) { return m-&gt;count(key) &gt; 0; }
bool h(std::map&lt;int, int&gt; *m, int key) { return m-&gt;count(key) == 0; }

&gt; ./build/bin/clang-tidy -checks="-*,readability-container-contains" tmp.cpp -- -std=c++20
8 warnings generated.
tmp.cpp:2:51: warning: use 'contains' to check for membership [readability-container-contains]
    2 | bool a(std::map&lt;int, int&gt; &amp;m, int key) { return m.find(key) != m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                   contains
tmp.cpp:3:51: warning: use 'contains' to check for membership [readability-container-contains]
    3 | bool b(std::map&lt;int, int&gt; &amp;m, int key) { return m.count(key) &gt; 0; }
      |                                                   ^~~~~      ~~~
      |                                                   contains
tmp.cpp:4:51: warning: use 'contains' to check for membership [readability-container-contains]
    4 | bool c(std::map&lt;int, int&gt; &amp;m, int key) { return m.find(key) == m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                 ! contains
tmp.cpp:5:51: warning: use 'contains' to check for membership [readability-container-contains]
    5 | bool d(std::map&lt;int, int&gt; &amp;m, int key) { return m.count(key) == 0; }
      |                                                   ^~~~~      ~~~~
      |                                                 ! contains
tmp.cpp:6:52: warning: use 'contains' to check for membership [readability-container-contains]
    6 | bool e(std::map&lt;int, int&gt; *m, int key) { return m-&gt;find(key) != m-&gt;end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                    contains
tmp.cpp:7:52: warning: use 'contains' to check for membership [readability-container-contains]
    7 | bool f(std::map&lt;int, int&gt; *m, int key) { return m-&gt;find(key) == m-&gt;end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                 !  contains
tmp.cpp:8:52: warning: use 'contains' to check for membership [readability-container-contains]
    8 | bool g(std::map&lt;int, int&gt; *m, int key) { return m-&gt;count(key) &gt; 0; }
      |                                                    ^~~~~      ~~~
      |                                                    contains
tmp.cpp:9:52: warning: use 'contains' to check for membership [readability-container-contains]
    9 | bool h(std::map&lt;int, int&gt; *m, int key) { return m-&gt;count(key) == 0; }
      |                                                    ^~~~~      ~~~~
      |                                                 !  contains

Full diff: https://github.com/llvm/llvm-project/pull/110386.diff

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp (+20-20)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp (+8-3)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index 05d4c87bc73cef..fb68c7d334b7f8 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -62,44 +62,44 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
                          .bind("positiveComparison"),
                      this);
   AddSimpleMatcher(
-      binaryOperator(hasOperatorName("!="), hasOperands(CountCall, Literal0))
+      binaryOperation(hasOperatorName("!="), hasOperands(CountCall, Literal0))
           .bind("positiveComparison"));
   AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
+      binaryOperation(hasLHS(CountCall), hasOperatorName(">"), hasRHS(Literal0))
           .bind("positiveComparison"));
   AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName(">="), hasRHS(Literal1))
-          .bind("positiveComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal1), hasOperatorName("<="), hasRHS(CountCall))
+      binaryOperation(hasLHS(Literal0), hasOperatorName("<"), hasRHS(CountCall))
           .bind("positiveComparison"));
+  AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName(">="),
+                                   hasRHS(Literal1))
+                       .bind("positiveComparison"));
+  AddSimpleMatcher(binaryOperation(hasLHS(Literal1), hasOperatorName("<="),
+                                   hasRHS(CountCall))
+                       .bind("positiveComparison"));
 
   // Find inverted membership tests which use `count()`.
   AddSimpleMatcher(
-      binaryOperator(hasOperatorName("=="), hasOperands(CountCall, Literal0))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("<="), hasRHS(Literal0))
-          .bind("negativeComparison"));
-  AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal0), hasOperatorName(">="), hasRHS(CountCall))
+      binaryOperation(hasOperatorName("=="), hasOperands(CountCall, Literal0))
           .bind("negativeComparison"));
+  AddSimpleMatcher(binaryOperation(hasLHS(CountCall), hasOperatorName("<="),
+                                   hasRHS(Literal0))
+                       .bind("negativeComparison"));
+  AddSimpleMatcher(binaryOperation(hasLHS(Literal0), hasOperatorName(">="),
+                                   hasRHS(CountCall))
+                       .bind("negativeComparison"));
   AddSimpleMatcher(
-      binaryOperator(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
+      binaryOperation(hasLHS(CountCall), hasOperatorName("<"), hasRHS(Literal1))
           .bind("negativeComparison"));
   AddSimpleMatcher(
-      binaryOperator(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
+      binaryOperation(hasLHS(Literal1), hasOperatorName(">"), hasRHS(CountCall))
           .bind("negativeComparison"));
 
   // Find membership tests based on `find() == end()`.
   AddSimpleMatcher(
-      binaryOperator(hasOperatorName("!="), hasOperands(FindCall, EndCall))
+      binaryOperation(hasOperatorName("!="), hasOperands(FindCall, EndCall))
           .bind("positiveComparison"));
   AddSimpleMatcher(
-      binaryOperator(hasOperatorName("=="), hasOperands(FindCall, EndCall))
+      binaryOperation(hasOperatorName("=="), hasOperands(FindCall, EndCall))
           .bind("negativeComparison"));
 }
 
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 9a9b233e07229b..f345b3e7768a8a 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
@@ -1,14 +1,19 @@
-// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t
+// RUN: %check_clang_tidy -std=c++11-or-later %s readability-container-contains %t
 
 // Some *very* simplified versions of `map` etc.
 namespace std {
 
 template <class Key, class T>
 struct map {
+  struct iterator {
+    bool operator==(const iterator &Other) const;
+    bool operator!=(const iterator &Other) const;
+  };
+
   unsigned count(const Key &K) const;
   bool contains(const Key &K) const;
-  void *find(const Key &K);
-  void *end();
+  iterator find(const Key &K);
+  iterator end();
 };
 
 template <class Key>

@EugeneZelenko
Copy link
Contributor

Please mention changes in Release Notes.

5chmidti
5chmidti approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicovank
[clang-tidy][readability-container-contains] Fix matching of non-bina…
e1dc2f8 
…ryOperator cases

Fix llvm#79437.

It works with non-mock `std::map`:
```
# All cases detailed in llvm#79437.
> cat tmp.cpp
#include <map>
bool a(std::map<int, int> &m, int key) { return m.find(key) != m.end(); }
bool b(std::map<int, int> &m, int key) { return m.count(key) > 0; }
bool c(std::map<int, int> &m, int key) { return m.find(key) == m.end(); }
bool d(std::map<int, int> &m, int key) { return m.count(key) == 0; }
bool e(std::map<int, int> *m, int key) { return m->find(key) != m->end(); }
bool f(std::map<int, int> *m, int key) { return m->find(key) == m->end(); }
bool g(std::map<int, int> *m, int key) { return m->count(key) > 0; }
bool h(std::map<int, int> *m, int key) { return m->count(key) == 0; }

> ./build/bin/clang-tidy -checks="-*,readability-container-contains" tmp.cpp -- -std=c++20
8 warnings generated.
tmp.cpp:2:51: warning: use 'contains' to check for membership [readability-container-contains]
    2 | bool a(std::map<int, int> &m, int key) { return m.find(key) != m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                   contains
tmp.cpp:3:51: warning: use 'contains' to check for membership [readability-container-contains]
    3 | bool b(std::map<int, int> &m, int key) { return m.count(key) > 0; }
      |                                                   ^~~~~      ~~~
      |                                                   contains
tmp.cpp:4:51: warning: use 'contains' to check for membership [readability-container-contains]
    4 | bool c(std::map<int, int> &m, int key) { return m.find(key) == m.end(); }
      |                                                   ^~~~      ~~~~~~~~~~
      |                                                 ! contains
tmp.cpp:5:51: warning: use 'contains' to check for membership [readability-container-contains]
    5 | bool d(std::map<int, int> &m, int key) { return m.count(key) == 0; }
      |                                                   ^~~~~      ~~~~
      |                                                 ! contains
tmp.cpp:6:52: warning: use 'contains' to check for membership [readability-container-contains]
    6 | bool e(std::map<int, int> *m, int key) { return m->find(key) != m->end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                    contains
tmp.cpp:7:52: warning: use 'contains' to check for membership [readability-container-contains]
    7 | bool f(std::map<int, int> *m, int key) { return m->find(key) == m->end(); }
      |                                                    ^~~~      ~~~~~~~~~~~
      |                                                 !  contains
tmp.cpp:8:52: warning: use 'contains' to check for membership [readability-container-contains]
    8 | bool g(std::map<int, int> *m, int key) { return m->count(key) > 0; }
      |                                                    ^~~~~      ~~~
      |                                                    contains
tmp.cpp:9:52: warning: use 'contains' to check for membership [readability-container-contains]
    9 | bool h(std::map<int, int> *m, int key) { return m->count(key) == 0; }
      |                                                    ^~~~~      ~~~~
      |                                                 !  contains
```
@nicovank nicovank mentioned this pull request Oct 23, 2024
Draft
@nicovank nicovank merged commit 3605d9a into llvm:main Oct 23, 2024
9 checks passed
@nicovank nicovank deleted the pr110386 branch October 23, 2024 18:57
@frobtech frobtech mentioned this pull request Oct 25, 2024
Closed
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
@nicovank @NoumanAmir657
[clang-tidy][readability-container-contains] Fix matching of non-bina…
7fb5211 
…ryOperator cases (llvm#110386)

Fix llvm#79437.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@5chmidti 5chmidti 5chmidti approved these changes

@PiotrZSL PiotrZSL Awaiting requested review from PiotrZSL

@SimplyDanny SimplyDanny Awaiting requested review from SimplyDanny

@HerrCai0907 HerrCai0907 Awaiting requested review from HerrCai0907

Assignees
No one assigned
Labels
clang-tidy clang-tools-extra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clang-tidy: checks "readability-container-contains" does not handle "find()"
4 participants
@nicovank @llvmbot @EugeneZelenko @5chmidti