[PATCH] D80023: [clang-tidy] Add abseil-string-find-str-contains checker.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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