avogelsgesang marked 2 inline comments as not done.
avogelsgesang added inline comments.
================
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;
----------------
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`?
================
Comment at:
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:110
+
+ // Diagnose the issue
+ auto Diag =
----------------
whisperity wrote:
> I'm not sure if these comments are useful, though. The business logic flow of
> the implementation is straightforward.
Agree, not sure how much value they provide.
Let me know if I should delete them, or if we want to keep them for the
structure they introduce...
================
Comment at:
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:113
+ diag(Call->getExprLoc(),
+ "use `contains` instead of `count` to check for containment");
+
----------------
whisperity wrote:
> whisperity wrote:
> > This might be a bit nitpicking, but `containment` sounds off here: it
> > usually comes up with regards to superset/subset relationships. Perhaps
> > phrasing in `membership` or `element` somehow would ease this.
> We use single apostrophe (`'`) instead of backtick (`) in the diagnostics for
> symbol names.
> but containment sounds off here
Agree. Thanks for catching this!
================
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.
+
----------------
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?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+
----------------
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"
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:111
+ // CHECK-FIXES: return M.count(21);
+}
----------------
whisperity wrote:
> Similarly, the test file could use at least some //negative// examples.
> Things like `count(X) >= 2` and such, to ensure that the matchers aren't
> inadvertently broken by someone which would result in a lot of false
> positives in production.
Added a couple of negative test cases. Please let me know if you have
additional additional test cases in mind which I should also add
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
