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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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