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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:49
+
+  // Find containment checks which use `count`
+  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
----------------
Same comment about "membership" or "element" instead of "containment" here.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:73
+
+  // Find inverted containment checks which use `count`
+  addSimpleMatcher(
----------------
Same comment about "membership" or "element" instead of "containment" here.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:93
+
+  // Find containment checks based on `begin == end`
+  addSimpleMatcher(
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:103
+void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
+  // Extract the ifnromation about the match
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:110
+
+  // Diagnose the issue
+  auto Diag =
----------------
I'm not sure if these comments are useful, though. The business logic flow of the implementation is straightforward.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:113
+      diag(Call->getExprLoc(),
+           "use `contains` instead of `count` to check for containment");
+
----------------
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.


================
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:
> 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.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h:19
+/// Finds usages of `container.count()` which should be replaced by a call
+/// to the `container.contains()` method introduced in C++ 20.
+///
----------------



================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:105-106
+
+  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.
+
----------------
Due to RST specifics, the code examples to be rendered as such, **double** backticks have to be used... Single backtick renders in a different font with emphasis (see example, blue), while double backtick is a proper monospace inline code snippet (see example, red).

{F20396099}


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:106
+  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.
+
----------------



================
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.
+
----------------
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.


================
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:
> 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.



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:111
+  // CHECK-FIXES: return M.count(21);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646