Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

nicovank
Copy link
Contributor

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 to modernize.
Possibly would like to use it as a codemod on our code.

Copy link

github-actions bot commented Nov 14, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@nicovank nicovank force-pushed the string-starts-with branch 2 times, most recently from 3abdf06 to dc890dc Compare November 14, 2023 16:54
@nicovank nicovank marked this pull request as ready for review November 14, 2023 17:42
@llvmbot
Copy link
Member

llvmbot commented Nov 14, 2023

@llvm/pr-subscribers-clang-tidy

Author: Nicolas van Kempen (nicovank)

Changes

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 to modernize.
Possibly would like to use it as a codemod on our code.


Full diff: https://github.com/llvm/llvm-project/pull/72283.diff

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp (+4-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/abseil/string-find-startswith.cpp (+21-2)
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);

@nicovank
Copy link
Contributor Author

First time with LLVM GitHub PR workflow. Am I supposed to tag potential reviewers? @gribozavr @hokein @njames93

@PiotrZSL
Copy link
Member

"I am slowly looking into adding an option to this check to recommend C++20's API starts_with, and move it to modernize.
Possibly would like to use it as a codemod on our code."

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.

Copy link
Member

@PiotrZSL PiotrZSL left a 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

Copy link
Member

@PiotrZSL PiotrZSL left a 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

@nicovank nicovank force-pushed the string-starts-with branch 3 times, most recently from 1761f02 to 42364eb Compare November 14, 2023 19:09
@nicovank
Copy link
Contributor Author

Copy this check.

Got it. Thanks for the pointer!

Documentation, string header.

I think I covered everything, had to add a couple of skeletons to that string common header.

@nicovank nicovank requested a review from PiotrZSL November 14, 2023 19:15
Copy link
Member

@PiotrZSL PiotrZSL left a 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.

@nicovank
Copy link
Contributor Author

Addressed feedback.

I do not have commit access, can you please merge after CI is green?
Assuming GitHub will properly populate information, if not you can use Nicolas van Kempen <[email protected]>.

PiotrZSL pushed a commit that referenced this pull request Nov 14, 2023
…t string-like classes (#72283)

As per title. A small improvement to this check, `string_view` should
work out of the box.
@PiotrZSL PiotrZSL closed this Nov 14, 2023
@nicovank nicovank deleted the string-starts-with branch November 14, 2023 22:28
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…t string-like classes (llvm#72283)

As per title. A small improvement to this check, `string_view` should
work out of the box.
PiotrZSL pushed a commit that referenced this pull request Dec 4, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants