Skip to content
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.

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

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

Merged
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

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

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

…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 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
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-tidy: checks "readability-container-contains" does not handle "find()"
4 participants