[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Thank you, this is getting much better!



================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108
+  const auto *PositiveCheck = Result.Nodes.getNodeAs<Expr>("positive");
+  const auto *NegativeCheck = Result.Nodes.getNodeAs<Expr>("negative");
+  bool Negated = NegativeCheck != nullptr;
+  const auto *Check = Negated ? NegativeCheck : PositiveCheck;
----------------
avogelsgesang wrote:
> whisperity wrote:
> > `Comparison` instead of `Check`? These should be matching the `binaryOperator`, right?
> In most cases, they match the `binaryOperator`. For the first pattern they match the `implicitCastExpr`, though
> 
> Given this is not always a `binaryOperator`, should I still rename it to `Comparison`?
`Comparison` is okay. Just `Check`, `Positive` and `Negative` seem a bit ambiguous at first glance.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst: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.
+
----------------
avogelsgesang wrote:
> whisperity wrote:
> > whisperity wrote:
> > > Same comment about the backtick count and how you would like the rendering to be. Please build the documentation locally and verify visually, as both ways most likely compile without warnings or errors.
> > 
> > Please build the documentation locally [...]
> 
> How do I actually do that?
You need to install [[ http://pypi.org/project/Sphinx | `Sphinx`]]. The easiest is to install it with //`pip`//, the Python package manager. Then, you tell LLVM CMake to create the build targets for the docs, and run it:

```lang=bash
~$ python3 -m venv sphinx-venv
~$ source ~/sphinx-venv/bin/activate
(sphinx-venv) ~$ pip install sphinx
(sphinx-venv) ~$ cd LLVM_BUILD_DIR
(sphinx-venv) LLVM_BUILD_DIR/$ cmake . -DLLVM_BUILD_DOCS=ON -DLLVM_ENABLE_SPHINX=ON
# configuration re-run
(sphinx-venv) LLVM_BUILD_DIR/$ cmake --build . -- docs-clang-tools-html
(sphinx-venv) LLVM_BUILD_DIR/$ cd ~
(sphinx-venv) ~$ deactivate
~$     # you are now back in HOME without the virtual env being active
```

The docs will be available starting from `${LLVM_BUILD_DIR}/tools/clang/tools/extra/docs/html/clang-tidy/index.html`.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:115
+
+// This is effectively a membership check, as the result is implicitely casted to `bool`
+bool returnContains(std::map<int, int> &M) {
----------------
Make sure to format it against 80-line, and also, we conventionally write full sentences in comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:129
+
+// Check that we are not confused by aliases
+namespace s2 = std;
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:174
+
+// The following map has the same interface like `std::map`
+template <class Key, class T>
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:189
+    // NO-WARNING.
+    // CHECK-FIXES: if (MyMap.count(0))
+    return nullptr;
----------------
If a fix is not generated, why is this line here?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+
----------------
avogelsgesang wrote:
> whisperity wrote:
> > Tests are to guard our future selves from breaking the system, so perhaps two tests that involve having `std::` typed out, and also using a different container that's not `std::whatever` would be useful.
> > 
> > ----
> > 
> > Do you think it would be worthwhile to add matching any user-defined object which has a `count()` and a `contains()` method (with the appropriate number of parameters) later? So match not only the few standard containers, but more stuff?
> > 
> > It doesn't have to be done now, but having a test for `MyContainer` not in `std::` being marked `// NO-WARNING.` or something could indicate that we purposefully don't want to go down that road just yet.
> > so perhaps two tests that involve having std:: typed out
> 
> rewrote the tests, such that most of them use fully-qualified types. Also added a few test cases involving type defs and namespace aliases (this actually uncovered a mistake in the matcher)
> 
> > Do you think it would be worthwhile to add matching any user-defined object which has a count() and a contains() method (with the appropriate number of parameters) later?
> 
> Not sure. At least not for the code base I wrote this check for...
> 
> > having a test for MyContainer not in std:: being marked // NO-WARNING. or something could indicate that we purposefully don't want to go down that road just yet
> 
> added such a test case and commented it as "not currently supported"
> 
>>! In D112646#3135409, @avogelsgesang wrote:
> added such a test case and commented it as "not currently supported"

Thank you! It will be enough for now, I believe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112646/new/

https://reviews.llvm.org/D112646