[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76496#1949851 , @niko wrote:

> Correct me if I'm wrong, but that seems to be in violation of IWYU? Maybe I'm 
> misreading this, or is the idea that higher-lever tooling (e.g. IWYU fixer) 
> is supposed to address that?


It is but it sort of gets a pass as the absl/strings/match.h has to include it. 
Similar to how if you `#include `, you wouldn't then `#include 
` so you could construct a `std::vector` using a 
`std::initializer_list`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76496



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Niko Weh via Phabricator via cfe-commits]

niko added a comment.

In D76496#1947127 , @njames93 wrote:

> In D76496#1947059 , @niko wrote:
>
> > In D76496#1935127 , @njames93 
> > wrote:
> >
> > > I'm not hugely familiar with the abseil library, but from what I can see 
> > > `absl::StartsWith` takes `absl::string_view` as args. Therefore surely it 
> > > makes sense to handle the 3rd arg for `str::find` by explicitly 
> > > constructing the `absl::string_view` from the pointer and size.
> >
> >
> > I am mainly worried that the 3-arg find is rarely used, and using 
> > absl::string_view requires another option to be specified for the header 
> > file. If you think that's still a good tradeoff i'll change it to construct 
> > a string_view.
>
>
> The `absl/strings/match` header includes the `absl/strings/string_view` 
> header so you wouldn't need to include another header file. Well that's as 
> long as the user hasn't done something funky with header files, but arguably 
> the shouldn't be using Fix-Its if they have done something like that.


Correct me if I'm wrong, but that seems to be in violation of IWYU? Maybe I'm 
misreading this, or is the idea that higher-lever tooling (e.g. IWYU fixer) is 
supposed to address that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76496



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76496#1947059 , @niko wrote:

> In D76496#1935127 , @njames93 wrote:
>
> > I'm not hugely familiar with the abseil library, but from what I can see 
> > `absl::StartsWith` takes `absl::string_view` as args. Therefore surely it 
> > makes sense to handle the 3rd arg for `str::find` by explicitly 
> > constructing the `absl::string_view` from the pointer and size.
>
>
> I am mainly worried that the 3-arg find is rarely used, and using 
> absl::string_view requires another option to be specified for the header 
> file. If you think that's still a good tradeoff i'll change it to construct a 
> string_view.


The `absl/strings/match` header includes the `absl/strings/string_view` header 
so you wouldn't need to include another header file. Well that's as long as the 
user hasn't done something funky with header files, but arguably the shouldn't 
be using Fix-Its if they have done something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76496



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Niko Weh via Phabricator via cfe-commits]

niko added a comment.

In D76496#1935127 , @njames93 wrote:

> I'm not hugely familiar with the abseil library, but from what I can see 
> `absl::StartsWith` takes `absl::string_view` as args. Therefore surely it 
> makes sense to handle the 3rd arg for `str::find` by explicitly constructing 
> the `absl::string_view` from the pointer and size.


I am mainly worried that the 3-arg find is rarely used, and using 
absl::string_view requires another option to be specified for the header file. 
If you think that's still a good tradeoff i'll change it to construct a 
string_view.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76496



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I'm not hugely familiar with the abseil library, but from what I can see 
`absl::StartsWith` takes `absl::string_view` as args. Therefore surely it makes 
sense to handle the 3rd arg for `str::find` by explicitly constructing the 
`absl::string_view` from the pointer and size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76496



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76496: [clang-tidy] StringFindStartswith should ignore 3-arg find()

[Niko Weh via Phabricator via cfe-commits]

niko created this revision.
niko added a reviewer: hokein.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

string::find() has a 3-argument version, with the third argument
specifying the 'length of sequence of characters to match' (for a const
char* 1st argument, typically used when it is not null-terminated).

This case seems rare enough that I think the best way is to just ignore
it. I believe the second-best alternative would be to pull in
string_view.h, and suggest constructing string_view from the first and
third argument - this seems like overkill but i'm happy to be convinced
otherwise.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76496

Files:
  clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
@@ -13,6 +13,7 @@
   ~basic_string();
   int find(basic_string s, int pos = 0);
   int find(const char *s, int pos = 0);
+  int find(const char *s, int pos, int n);
 };
 typedef basic_string string;
 typedef basic_string wstring;
@@ -41,6 +42,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
 
+  s.find(s, 0) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
   s.find("aaa") != 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
@@ -66,4 +71,5 @@
   s.find("a", 1) == 0;
   s.find("a", 1) == 1;
   s.find("a") == 1;
+  s.find("a", 0, 1) == 0;
 }
Index: clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
@@ -40,8 +40,9 @@
 
   auto StringFind = cxxMemberCallExpr(
   // .find()-call on a string...
-  callee(cxxMethodDecl(hasName("find"))),
-  on(hasType(StringType)),
+  callee(cxxMethodDecl(hasName("find"))), on(hasType(StringType)),
+  // ... with 1 or two arguments (but not three),
+  anyOf(argumentCountIs(1), argumentCountIs(2)),
   // ... with some search expression ...
   hasArgument(0, expr().bind("needle")),
   // ... and either "0" as second argument or the default argument (also 
0).


Index: clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp
@@ -13,6 +13,7 @@
   ~basic_string();
   int find(basic_string s, int pos = 0);
   int find(const char *s, int pos = 0);
+  int find(const char *s, int pos, int n);
 };
 typedef basic_string string;
 typedef basic_string wstring;
@@ -41,6 +42,10 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
 
+  s.find(s, 0) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
   s.find("aaa") != 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
   // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
@@ -66,4 +71,5 @@
   s.find("a", 1) == 0;
   s.find("a", 1) == 1;
   s.find("a") == 1;
+  s.find("a", 0, 1) == 0;
 }
Index: clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
===
--- clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
@@ -40,8 +40,9 @@
 
   auto StringFind = cxxMemberCallExpr(
   // .find()-call on a string...
-  callee(cxxMethodDecl(hasName("find"))),
-  on(hasType(StringType)),
+  callee(cxxMethodDecl(hasName("find"))), on(hasType(StringType)),
+  // ... with 1 or two arguments (but not three),
+  anyOf(argumentCountIs(1), argumentCountIs(2)),
   // ... with some search expression ...
   hasArgument(0, expr().bind("needle")),
   // ... and either "0" as second argument or the default argument (also 0).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits