[clang-tidy] support string::contains #110351

dl8sd11 · 2024-09-28T07:44:36Z

Starting from c++23, we can replace std::string::find() == std::string::npos with std::string.contains() . #109327

Currently, this is WIP because there are two limitations:

False positive: SubStr type is const std::string& which does not match std::string::contains(basic_string_view) type.

std::string SubStr;
  if (Str.find(SubStr) != std::string::npos) {};

Currently, the fixit for std::string::find("test", 0) is incorrect.

github-actions · 2024-09-28T07:44:54Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

nicovank

IMO no need to update release notes:

Improved readability-container-contains check to let it work on any class that has a contains method.

But I think an example should be added to the check documentation.

nicovank · 2024-09-28T20:23:13Z

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp

@@ -32,7 +33,8 @@ void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {

  const auto FindCall =
      cxxMemberCallExpr(
-          argumentCountIs(1),
+          anyOf(argumentCountIs(1),
+                allOf(argumentCountIs(2), hasArgument(1, cxxDefaultArgExpr()))),


Maybe a comment here saying std::string and std::string_view take an optional pos argument on where to start the search. Also match ignoringParenImpCasts(integerLiteral(0))?

I tried to match the two arguments but I fail to remove the second argument in the Fixit hint. Could you suggest how to get the location after the first argument. I tried binding the first argument (e.g. "test"), but the source range seems to be the first character (") instead of the whole argument ("test").

Considering str.find('a') != std::string::npos. Instead of deleting stuff from ) to npos, you could replace everything from after 'a' to the end of the comparison with ), that should work. I wrote something like this here:

llvm-project/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp

Lines 159 to 165 in e8f01b0

// Remove possible arguments after search expression and ' [!=]= 0' suffix.

Diagnostic << FixItHint::CreateReplacement(

CharSourceRange::getTokenRange(

Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,

*Result.SourceManager, getLangOpts()),

ComparisonExpr->getEndLoc()),

")");

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

nicovank · 2024-10-05T15:13:35Z

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string

+  bool contains(const C *s) const;
+  bool contains(C s) const;


Suggested change

bool contains(const C *s) const;

bool contains(C s) const;

bool contains(const C *s) const;

bool contains(C s) const;

bool contains(basic_string_view<C, T> sv) const;

nicovank · 2024-10-05T15:13:59Z

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string

+  bool contains(const C *s) const;
+  bool contains(C s) const;


Suggested change

bool contains(const C *s) const;

bool contains(C s) const;

bool contains(const C *s) const;

bool contains(C s) const;

bool contains(basic_string_view sv) const;

nicovank · 2024-10-05T15:17:18Z

clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp

  const auto FindCall =
      cxxMemberCallExpr(
-          argumentCountIs(1),
+          anyOf(argumentCountIs(1),
+                allOf(argumentCountIs(2), // string::find takes two arguments


Suggested change

allOf(argumentCountIs(2), // string::find takes two arguments

// `find` for string-like classes often has a second parameter,

// indicating where to start the search. We match if it's

// the default zero or explicitly equal to zero.

allOf(argumentCountIs(2),

I think a little more detail is good here.

nicovank · 2024-10-05T15:23:25Z

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string

@@ -50,12 +50,16 @@ struct basic_string {
  size_type find(const _Type& str, size_type pos = 0) const;
  size_type find(const C* s, size_type pos = 0) const;
  size_type find(const C* s, size_type pos, size_type n) const;
+  size_type find(C ch, size_type pos = 0) const;


Suggested change

size_type find(C ch, size_type pos = 0) const;

size_type find(C ch, size_type pos = 0) const;

template<class StringViewLike>

size_type find(const StringViewLike& t, size_type pos = 0) const;

Let's cover them all if possible.
This one might cause issues, and is probably not all that common, but let's see.

nicovank · 2024-10-05T17:06:53Z

clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

@@ -453,3 +458,29 @@ void testOperandPermutations(std::map<int, int>& Map) {
  // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use 'contains' to check for membership [readability-container-contains]
  // CHECK-FIXES: if (!Map.contains(0)) {};
 }
+
+void testStringNops(std::string Str, std::string SubStr, std::string_view StrView) {


Suggested change

void testStringNops(std::string Str, std::string SubStr, std::string_view StrView) {

void testStringNpos(std::string Str, std::string SubStr, std::string_view StrView) {

Just a typo.

nicovank · 2024-10-05T17:37:38Z

Thanks!
Is there still an issue with type stuff? If so, what would be a test case that fails that shouldn't?

nicovank · 2024-10-23T18:56:44Z

FYI I'm going to merge #110386 now which will cause conflicts (binaryOperator -> binaryOperation).

[clang-tidy] support string::contains

b98e9a4

nicovank reviewed Sep 28, 2024

View reviewed changes

dl8sd11 added 4 commits October 5, 2024 04:26

[clang-tidy] fix string::find(s, 0)

e42c356

[clang-tidy] add string::contains

9d44ee0

[clang-tidy] adopt mock headers and add string_view tests

3625221

[clang-tidy] add comment expain 2 arguments

608e883

dl8sd11 requested a review from nicovank October 5, 2024 04:35

nicovank reviewed Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-tidy] support string::contains #110351

[clang-tidy] support string::contains #110351

dl8sd11 commented Sep 28, 2024

github-actions bot commented Sep 28, 2024

nicovank left a comment

nicovank Sep 28, 2024

dl8sd11 Oct 5, 2024

nicovank Oct 5, 2024

nicovank Oct 5, 2024

nicovank Oct 5, 2024

nicovank Oct 5, 2024

nicovank Oct 5, 2024

nicovank Oct 5, 2024

nicovank commented Oct 5, 2024

nicovank commented Oct 23, 2024

	// Remove possible arguments after search expression and ' [!=]= 0' suffix.
	Diagnostic << FixItHint::CreateReplacement(
	CharSourceRange::getTokenRange(
	Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
	*Result.SourceManager, getLangOpts()),
	ComparisonExpr->getEndLoc()),
	")");

-                allOf(argumentCountIs(2), // string::find takes two arguments
+                // `find` for string-like classes often has a second parameter,
+                // indicating where to start the search. We match if it's
+                // the default zero or explicitly equal to zero.
+                allOf(argumentCountIs(2),

	void testStringNops(std::string Str, std::string SubStr, std::string_view StrView) {
	void testStringNpos(std::string Str, std::string SubStr, std::string_view StrView) {

[clang-tidy] support string::contains #110351

Are you sure you want to change the base?

[clang-tidy] support string::contains #110351

Conversation

dl8sd11 commented Sep 28, 2024

github-actions bot commented Sep 28, 2024

nicovank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicovank commented Oct 5, 2024

nicovank commented Oct 23, 2024