-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang-tidy][abseil-string-find-startswith] Add string_view to default string-like classes #72283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
Outdated
Show resolved
Hide resolved
✅ With the latest revision this PR passed the C/C++ code formatter. |
3abdf06
to
dc890dc
Compare
@llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) ChangesAs per title. A small improvement to this check, I am slowly looking into adding an option to this check to recommend C++20's API Full diff: https://github.com/llvm/llvm-project/pull/72283.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
index b36144d1912fce0..d0527cba22a917d 100644
--- a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
@@ -19,11 +19,14 @@ using namespace clang::ast_matchers;
namespace clang::tidy::abseil {
+const auto DefaultStringLikeClasses =
+ "::std::basic_string; ::std::basic_string_view";
+
StringFindStartswithCheck::StringFindStartswithCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
StringLikeClasses(utils::options::parseStringList(
- Options.get("StringLikeClasses", "::std::basic_string"))),
+ Options.get("StringLikeClasses", DefaultStringLikeClasses))),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
diff --git a/clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp b/clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
index e51568077eda151..ae8d2daf736e979 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
@@ -1,5 +1,5 @@
// RUN: %check_clang_tidy %s abseil-string-find-startswith %t -- \
-// RUN: -config="{CheckOptions: {abseil-string-find-startswith.StringLikeClasses: '::std::basic_string;::basic_string'}}"
+// RUN: -config="{CheckOptions: {abseil-string-find-startswith.StringLikeClasses: '::std::basic_string;::std::basic_string_view;::basic_string'}}"
using size_t = decltype(sizeof(int));
@@ -22,6 +22,21 @@ struct basic_string {
typedef basic_string<char> string;
typedef basic_string<wchar_t> wstring;
+template <typename C>
+class basic_string_view {
+public:
+ basic_string_view();
+ basic_string_view(const basic_string_view &);
+ basic_string_view(const C *);
+ ~basic_string_view();
+ int find(basic_string_view s, int pos = 0);
+ int find(const C *s, int pos = 0);
+ int find(const C *s, int pos, int n);
+ int find(char c, int pos = 0);
+ static constexpr size_t npos = -1;
+};
+typedef basic_string_view<char> string_view;
+
struct cxx_string {
int find(const char *s, int pos = 0);
int rfind(const char *s, int pos = npos);
@@ -39,7 +54,7 @@ std::string bar();
#define A_MACRO(x, y) ((x) == (y))
-void tests(std::string s, global_string s2) {
+void tests(std::string s, global_string s2, std::string_view sv) {
s.find("a") == 0;
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
// CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
@@ -96,6 +111,10 @@ void tests(std::string s, global_string s2) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
// CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s2, "a");{{$}}
+ sv.find("a") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+ // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(sv, "a");{{$}}
+
// expressions that don't trigger the check are here.
A_MACRO(s.find("a"), 0);
A_MACRO(s.rfind("a", 0), 0);
|
First time with LLVM GitHub PR workflow. Am I supposed to tag potential reviewers? @gribozavr @hokein @njames93 |
clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
Outdated
Show resolved
Hide resolved
"I am slowly looking into adding an option to this check to recommend C++20's API starts_with, and move it to modernize. Actually best would be to copy this check to performance-use-starts_with (or modernize) or something like this, and restrict it to C++20, and this check restrict to !C++20., But similar case is with ends_with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for this check:
- use string header
- update check documentation (option default and description)
- make sure that documentation is in sync with class documentation
- add release notes entry about improving check to support std::string_view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation, string header, release notes
1761f02
to
42364eb
Compare
Got it. Thanks for the pointer!
I think I covered everything, had to add a couple of skeletons to that |
clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/abseil/string-find-startswith.rst
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, once you correct latest comments & CI will pass, fell free to merge.
…t string-like classes
42364eb
to
cf4a78c
Compare
Addressed feedback. I do not have commit access, can you please merge after CI is green? |
…t string-like classes (#72283) As per title. A small improvement to this check, `string_view` should work out of the box.
…t string-like classes (llvm#72283) As per title. A small improvement to this check, `string_view` should work out of the box.
Make a modernize version of abseil-string-find-startswith using the available C++20 `std::string::starts_with` and `std::string_view::starts_with`. Following up from #72283.
As per title. A small improvement to this check,
string_view
should work out of the box.I am slowly looking into adding an option to this check to recommend C++20's API
starts_with
, and move it tomodernize
.Possibly would like to use it as a codemod on our code.