ymandel added a comment.
Looks good, just some nits!
================
Comment at:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:48
+ Options.get("StringLikeClasses", DefaultStringLikeClasses));
+ const std::string AbseilStringsMatchHeader(
+ Options.get("AbseilStringsMatchHeader",
DefaultAbseilStringsMatchHeader));
----------------
nit: just use assignment? `Options.get()` returns a string, so `=` seems a
clearer way to initialize.
================
Comment at:
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:72
+ binaryOperator(hasOperatorName("=="),
+ hasEitherOperand(ignoringParenImpCasts(StringNpos)),
+ hasEitherOperand(ignoringParenImpCasts(StringFind))),
----------------
Would `hasOperands` replace these two separate calls to `hasEitherOperand`?
Same below lines 79-80 (I think it's a new matcher, fwiw).
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst:6
+
+Finds s.find(...) == string::npos comparisons (for various string-like types)
+and suggests replacing with absl::StrContains.
----------------
nit: use double back ticks (like below)
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst:7
+Finds s.find(...) == string::npos comparisons (for various string-like types)
+and suggests replacing with absl::StrContains.
+
----------------
Same for `absl::StrContains`.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst:28
+
+ string s = "...";
+ if (!absl::StrContains(s, "Hello World")) { /* do something */ }
----------------
nit: `std::string`
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp:2
+// RUN: %check_clang_tidy %s abseil-string-find-str-contains %t -- \
+// RUN: -config="{CheckOptions: []}"
+
----------------
I'm not sure what's standard practice, but consider whether its worth adding
another test file that tests the check options. It wouldn't have to be
comprehensive like this one, just some basic tests that the options are being
honored.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80023/new/
https://reviews.llvm.org/D80023
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
