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

[Tom Lokovic via Phabricator via cfe-commits]

tdl-g added a comment.

Thanks, all, for the additional comments.  I addressed them all except for the suggestion to add an options-specific test.  I'm not against it, but (as I mention in the comment) I'm also unsure how to meaningfully test the include-inserting-related options.



================
Comment at: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:72
+           binaryOperator(hasOperatorName("=="),
+                          hasEitherOperand(ignoringParenImpCasts(StringNpos)),
+                          hasEitherOperand(ignoringParenImpCasts(StringFind))),
----------------
njames93 wrote:
> ymandel wrote:
> > Would `hasOperands` replace these two separate calls to `hasEitherOperand`? Same below lines 79-80 (I think it's a new matcher, fwiw).
> Just added, I added it to remove calls of multiple hasEitherOperand calls for the reason of preventing the matchers matching the same operand. I just got a little lost in my previous comment
Switched to hasOperands.  I agree that expresses the intent better than two independent calls of hasEitherOperand.

Note that it was working fine before--the test cases confirmed that it wasn't matching string::npos == string::npos or s.find(a) == s.find(a).  I'm guessing that's because the matchers inside hasEitherOperand have bindings, and if the matcher matched but one of the bindings was missing, the rewriterule suppressed itself?  Is this a plausible theory?

(The question moot since hasOperands is better overall, but I'd like to understand why it was working before.)


================
Comment at: clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp:98
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus;
+}
----------------
njames93 wrote:
> nit: as abseil requires c++11, should this check also require c++11 support
Done, though I note that none of the other checkers in the abseil directory are checking for CplusCplus11.  (That's not an argument against doing it, just a question of whether or not they should also get this change at some point.)


================
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: []}"
+
----------------
ymandel wrote:
> 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.
I'm open to it, but note that two of the three options (IncludeStyle, inherited from TransformerClangTidy) and AbseilStringsMatchHeader, only manifest in the header-inclusion.

So if I had a separate test it would be easy to confirm that StringLikeClasses was being honored.  But I'm not sure how I'd confirm that IncludeStyle or AbseilStringsMatchHeader are being honored.  Suggestions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80023