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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
