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