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