Page MenuHomePhabricator

[clang-tidy] Add abseil-string-find-str-contains checker.
ClosedPublic

Authored by tdl-g on May 15 2020, 10:57 AM.

Details

Summary

This adds a checker which suggests replacing string.find(...) == npos with absl::StrContains.

Diff Detail

Event Timeline

tdl-g created this revision.May 15 2020, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2020, 10:57 AM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko retitled this revision from Add abseil-string-find-str-contains checker. to [clang-tidy] Add abseil-string-find-str-contains checker..
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
19

asserts are not used.

36

Please use static instead of anonymous namespace for functions.

39

This belongs to isLanguageVersionSupported().

clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.h
17

STL containers are not used in header.

33

storeOptions() is missing.

clang-tools-extra/docs/ReleaseNotes.rst
78

Please separate with empty line.

81

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/list.rst
23

Unrelated and incorrect changes.

Eugene, thank you for the comments, I'll address them soon. For the moment I'm trying to figure out what's up with the list.rst changes.

clang-tools-extra/docs/clang-tidy/checks/list.rst
23

Agreed, these were generated by "clang-tidy/add_new_check.py abseil string-find-str-contains", still trying to figure out why.

njames93 added inline comments.May 15 2020, 1:46 PM
clang-tools-extra/docs/clang-tidy/checks/list.rst
23

Did you invoke the python script from the clang-tidy folder or the clang-tools-extra folder. The script is sensitive to the working directory it was executed from, it must be executed from the clang-tidy folder otherwise it fails when trying to detect auto fixes.

njames93 added inline comments.May 15 2020, 6:27 PM
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
2–3

Don't need the C++ specifier in a cpp file.

//===--- StringFindStrContainsCheck.cpp - clang-tidy ----------------------===//
72–73

This is dangerous, it will match on std::string::npos == std::string::npos and (more importantly) strA.find(...) == strB.find(...). See D80054.

79–80

Same as above.

njames93 added inline comments.May 15 2020, 6:29 PM
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
72–73

Ignore me, its late.

tdl-g updated this revision to Diff 264986.May 19 2020, 12:02 PM
tdl-g marked 16 inline comments as done.

Addressed review comments.

Thanks, all for the comments. I believe I've addressed all comments. Note that TransformerClangTidyCheck interacts awkwardly with StoreOptions; I have a FIXME to clean that up.

clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
72–73

Just to be safe, I added a test case that confirms that it doesn't match on those constructs.

clang-tools-extra/docs/clang-tidy/checks/list.rst
23

Possibly. I re-ran it on a clean cloned repo from the clang-tidy folder and it still created some unrelated list.rst changes (though fewer than before). So for now I just added the one line
I want by hand.

clang-tools-extra/docs/ReleaseNotes.rst
82

Please use double back-ticks to highlight language constructs. Same in documentation.

njames93 added inline comments.May 19 2020, 12:21 PM
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
102–103

I don't know if you have rebased recently, but TransformerClangTidyCheck now has a storeOptions method that needs calling from derived classes that override storeOptions. For now can you add a call to that in here TransformerClangTidyCheck::storeOptions(Opts); I Agree that we need to rework how options are handled but for now, make it work correctly.

Looks good, just some nits!

clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
49

nit: just use assignment? Options.get() returns a string, so = seems a clearer way to initialize.

73

Would hasOperands replace these two separate calls to hasEitherOperand? Same below lines 79-80 (I think it's a new matcher, fwiw).

clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
7

nit: use double back ticks (like below)

8

Same for absl::StrContains.

29

nit: std::string

clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
3

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.

njames93 added inline comments.May 19 2020, 2:33 PM
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
73

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

99

nit: as abseil requires c++11, should this check also require c++11 support

tdl-g updated this revision to Diff 265234.May 20 2020, 7:25 AM
tdl-g marked 12 inline comments as done.

Addressed second round of comments.

tdl-g added a comment.May 20 2020, 7:26 AM

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.

clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
73

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.)

99

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.)

clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
3

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?

LGTM, but leaving for one of the other reviewers to give you the official "Accept Revision".

clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
73

I was wondering the same when I noticed this. I think its something like this: one of the arguments falls into bucket 1, one of the arguments falls into bucket 2. Since a single argument cannot simultaneously fall into two buckets (the predicates are mutually exlusive), it must be the case that the arguments are different.

Eugene.Zelenko added inline comments.May 20 2020, 9:34 AM
clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-str-contains.rst
5

Please make same length as title.

tdl-g updated this revision to Diff 266207.May 26 2020, 7:41 AM

Fixed length of visual separator.

tdl-g marked 4 inline comments as done.May 26 2020, 7:45 AM

Thanks again, addressed all comments.

clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-str-contains.cpp
3

Discussed with ymandel offline, we'll add more option-specific tests when we clean up transformer's options handling.

njames93 added inline comments.May 28 2020, 1:17 AM
clang-tools-extra/clang-tidy/abseil/StringFindStrContainsCheck.cpp
41–45

I've put in a patch to hopefully address this shortcoming D80697. Don't worry about this right now, but If that lands then it will make this a lot cleaner.

ymandel accepted this revision.May 28 2020, 7:14 AM
This revision is now accepted and ready to land.May 28 2020, 7:14 AM
This revision was automatically updated to reflect the committed changes.

We see this broke the build for shared-lib config http://lab.llvm.org:8011/builders/llvm-avr-linux/builds/1879

Looking now (and I look forward to determining what I should have done to avoid this).