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: checks "readability-container-contains" does not handle "find()" #79437

Closed
adesitter opened this issue Jan 25, 2024 · 3 comments · Fixed by #110386
Closed

clang-tidy: checks "readability-container-contains" does not handle "find()" #79437

adesitter opened this issue Jan 25, 2024 · 3 comments · Fixed by #110386
Labels
clang-tidy confirmed Verified by a second party

Comments

@adesitter
Copy link

adesitter commented Jan 25, 2024

$ cat qq.cpp

#include <map>
bool my_contains_1(std::map<int, int> const& m, int key)
{
   return m.find(key) != m.end(); 
}
bool my_contains_2(std::map<int, int> const& m, int key)
{
   return m.count(key) > 0; 
}

$ /usr/local/clang-17/bin/clang-tidy '-checks=readability-container-contains' contains.cpp -- -std=c++20
1 warning generated.
/tmp/contains.cpp:8:13: warning: use 'contains' to check for membership [readability-container-contains]
8 | return m.count(key) > 0;
| ^~~~~ ~~~
| contains

Despite https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html, readability-container-contains" does not handle "find()".

@PiotrZSL PiotrZSL added the confirmed Verified by a second party label Jan 25, 2024
@PiotrZSL
Copy link
Member

Interesting, in C++20 this code looks like this:

 `-CXXRewrittenBinaryOperator <col:11, col:32> 'bool'
|         `-UnaryOperator <col:23, col:32> 'bool' prefix '!' cannot overflow
|           `-CXXOperatorCallExpr <col:11, col:32> 'bool' '==' adl

when in C++17:

CXXOperatorCallExpr <col:11, col:32> 'bool' '!=' adl

And check support only binaryOperator, so it won't work on iterators objects. Looks like support for those 2 need simply be added.

@adesitter
Copy link
Author

adesitter commented Jan 25, 2024

readability-container-contains is only relevant in C++20 mode.
operator== is equally affected:

$ cat contains2.cpp 
#include <map>
bool my_not_contains_1(std::map<int, int> const& m, int key)
{
   return m.find(key) == m.end(); 
}
bool my_not_contains_2(std::map<int, int> const& m, int key)
{
   return m.count(key) == 0; 
}
$ /usr/local/clang-17/bin/clang-tidy '-checks=readability-container-contains' contains2.cpp -- -std=c++20
1 warning generated.
/tmp/contains2.cpp:8:13: warning: use 'contains' to check for membership [readability-container-contains]
    8 |    return m.count(key) == 0; 
      |             ^~~~~      ~~~~
      |           ! contains

@adesitter
Copy link
Author

adesitter commented Jan 31, 2024

readability-container-contains does not handle pointer dereferencing.

$ cat contains.cpp 
#include <map>
bool my_contains_1(std::map<int, int> const* m, int key)
{
   return m->find(key) != m->end(); 
}
bool my_not_contains_1(std::map<int, int> const* m, int key)
{
   return m->find(key) == m->end(); 
}
bool my_contains_2(std::map<int, int> const* m, int key)
{
   return m->count(key) > 0; 
}
bool my_not_contains_2(std::map<int, int> const* m, int key)
{
   return m->count(key) == 0; 
}
$ /usr/local/clang-17/bin/clang-tidy '-checks=readability-container-contains' contains.cpp -- -std=c++20

nicovank added a commit to nicovank/llvm-project that referenced this issue Sep 28, 2024
…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 added a commit to nicovank/llvm-project that referenced this issue Sep 29, 2024
…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 added a commit to nicovank/llvm-project that referenced this issue Oct 22, 2024
…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
```
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this issue Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang-tidy confirmed Verified by a second party
Projects
None yet
2 participants