[clang-tidy] Add a new check 'replace-with-string-view' #172170
+334
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Looks for functions returning `std::[w|u8|u16|u32]string` and suggests to
change it to `std::[...]string_view` if possible and profitable.
Example:
std::string foo(int i) { // <---- can be replaced to `std::string_view f(...) {`
switch(i) {
case 1:
return "case1";
case 2:
return "case2";
default:
return {};
}
}
Member
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Zinovy Nis (irishrover) ChangesLooks for functions returning Example: std::string foo(int i) { // <---- can be replaced to Full diff: https://github.com/llvm/llvm-project/pull/172170.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index 9a2f90069edbf..21a9442be69c0 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
NoexceptMoveConstructorCheck.cpp
NoexceptSwapCheck.cpp
PerformanceTidyModule.cpp
+ ReplaceWithStringViewCheck.cpp
TriviallyDestructibleCheck.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitializationCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 3497ea7316c6b..e17bfc42be58f 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -24,6 +24,7 @@
#include "NoexceptDestructorCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "NoexceptSwapCheck.h"
+#include "ReplaceWithStringViewCheck.h"
#include "TriviallyDestructibleCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitializationCheck.h"
@@ -62,6 +63,8 @@ class PerformanceModule : public ClangTidyModule {
"performance-noexcept-move-constructor");
CheckFactories.registerCheck<NoexceptSwapCheck>(
"performance-noexcept-swap");
+ CheckFactories.registerCheck<ReplaceWithStringViewCheck>(
+ "performance-replace-with-string-view");
CheckFactories.registerCheck<TriviallyDestructibleCheck>(
"performance-trivially-destructible");
CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
diff --git a/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.cpp b/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.cpp
new file mode 100644
index 0000000000000..0cb11cda8e1aa
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.cpp
@@ -0,0 +1,74 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReplaceWithStringViewCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+namespace {
+llvm::StringLiteral toStringViewTypeStr(StringRef Type) {
+ if (Type.contains("wchar_t"))
+ return "std::wstring_view";
+ if (Type.contains("wchar_t"))
+ return "std::wstring_view";
+ if (Type.contains("char8_t"))
+ return "std::u8string_view";
+ if (Type.contains("char16_t"))
+ return "std::u16string_view";
+ if (Type.contains("char32_t"))
+ return "std::u32string_view";
+ return "std::string_view";
+}
+} // namespace
+
+void ReplaceWithStringViewCheck::registerMatchers(MatchFinder *Finder) {
+ const auto IsStdString = hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(hasName("::std::basic_string"))));
+ const auto IsStdStringView = expr(hasType(hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view"))))));
+
+ Finder->addMatcher(
+ functionDecl(
+ isDefinition(), returns(IsStdString), hasDescendant(returnStmt()),
+ unless(hasDescendant(
+ returnStmt(hasReturnValue(unless(ignoringImplicit(anyOf(
+ stringLiteral(), IsStdStringView,
+ cxxConstructExpr(
+ hasType(IsStdString),
+ anyOf(argumentCountIs(0),
+ hasArgument(0, ignoringParenImpCasts(anyOf(
+ stringLiteral(),
+ IsStdStringView)))))))))))))
+ .bind("func"),
+ this);
+}
+
+void ReplaceWithStringViewCheck::check(const MatchFinder::MatchResult &Result) {
+ if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("func")) {
+ const llvm::StringLiteral DestReturnTypeStr =
+ toStringViewTypeStr(MatchedDecl->getReturnType()
+ .getDesugaredType(*Result.Context)
+ .getAsString());
+
+ auto Diag = diag(MatchedDecl->getReturnTypeSourceRange().getBegin(),
+ "consider using `%0' to avoid unnecessary "
+ "copying and allocations")
+ << DestReturnTypeStr << MatchedDecl;
+
+ for (const auto *FuncDecl = MatchedDecl; FuncDecl != nullptr;
+ FuncDecl = FuncDecl->getPreviousDecl()) {
+ Diag << FixItHint::CreateReplacement(FuncDecl->getReturnTypeSourceRange(),
+ DestReturnTypeStr);
+ }
+ }
+}
+
+} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.h b/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.h
new file mode 100644
index 0000000000000..a9635939e850d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.h
@@ -0,0 +1,47 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_REPLACEWITHSTRINGVIEWCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_REPLACEWITHSTRINGVIEWCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::performance {
+
+/// Looks for functions returning `std::[w|u8|u16|u32]string` and suggests to
+/// change it to `std::[...]string_view` if possible and profitable.
+///
+/// Example:
+///
+/// std::string foo(int i) { // <---- can be replaced to std::string_view f(...)
+/// switch(i) {
+/// case 1:
+/// return "case1";
+/// case 2:
+/// return "case2";
+/// default:
+/// return {};
+/// }
+/// }
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/performance/replace-with-string-view.html
+class ReplaceWithStringViewCheck : public ClangTidyCheck {
+public:
+ ReplaceWithStringViewCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus17;
+ }
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_REPLACEWITHSTRINGVIEWCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 27467d082ea1e..03470b8c78489 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -244,6 +244,12 @@ New checks
Finds virtual function overrides with different visibility than the function
in the base class.
+- New :doc:`performance-replace-with-string-view
+ <clang-tidy/checks/performance/replace-with-string-view>` check.
+
+ Detects functions returning std::[w|u8|u16|u32]string where
+ return type can be changed to std::[...]string_view.
+
- New :doc:`readability-redundant-parentheses
<clang-tidy/checks/readability/redundant-parentheses>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e5e77b5cc418b..a45293b1f7823 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -362,6 +362,7 @@ Clang-Tidy Checks
:doc:`performance-noexcept-destructor <performance/noexcept-destructor>`, "Yes"
:doc:`performance-noexcept-move-constructor <performance/noexcept-move-constructor>`, "Yes"
:doc:`performance-noexcept-swap <performance/noexcept-swap>`, "Yes"
+ :doc:`performance-replace-with-string-view <performance/replace-with-string-view>`, "Yes"
:doc:`performance-trivially-destructible <performance/trivially-destructible>`, "Yes"
:doc:`performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn>`, "Yes"
:doc:`performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/replace-with-string-view.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/replace-with-string-view.rst
new file mode 100644
index 0000000000000..a5dc48b450c40
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/replace-with-string-view.rst
@@ -0,0 +1,60 @@
+.. title:: clang-tidy - performance-replace-with-string-view
+
+performance-replace-with-string-view
+====================================
+
+Looks for functions returning `std::[w|u8|u16|u32]string` and suggests to
+change it to `std::[...]string_view` for performance reasons.
+
+Rationale:
+
+Each time a new ``std::string`` is created from a literal, a copy of that
+literal is allocated either in ``std::string``'s internal buffer
+(for short literals) or in a heap.
+
+For the cases where ``std::string`` is returned from a function,
+such allocations can be eliminated sometimes by using std::string_view
+as a return type.
+
+This check looks for such functions returning ``std::string``
+baked from the literals and suggests replacing the return type to ``std::string_view``.
+
+It handles std::string,std::wstring, std::u8string, std::u16string and
+std::u16string and properly selects the kind of std::string_view to return.
+Typedefs for these string type also supported.
+
+Example:
+
+Consider the following code:
+
+.. code-block:: c++
+
+ std::string foo(int i) {
+ switch(i)
+ {
+ case 1: return "case 1";
+ case 2: return "case 2";
+ case 3: return "case 3";
+ default: return "default";
+ }
+ }
+
+In the code above a new ``std::string`` object is created on each
+function invocation, making a copy of a string literal and possibly
+allocating a memory in a heap.
+
+The check gets this code transformed into:
+
+.. code-block:: c++
+
+ std::string_view foo(int i) {
+ switch(i)
+ {
+ case 1: return "case 1";
+ case 2: return "case 2";
+ case 3: return "case 3";
+ default: return "default";
+ }
+ }
+
+New version re-uses statically allocated literals without additional overhead.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/replace-with-string-view.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/replace-with-string-view.cpp
new file mode 100644
index 0000000000000..625d729a6e4c9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/replace-with-string-view.cpp
@@ -0,0 +1,142 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-replace-with-string-view %t -- -- -I %S/Inputs
+
+namespace std {
+ template <typename CharT>
+ class basic_string_view {
+ public:
+ basic_string_view(const CharT *);
+ basic_string_view();
+ };
+ using string_view = basic_string_view<char>;
+ using wstring_view = basic_string_view<wchar_t>;
+ using u16string_view = basic_string_view<char16_t>;
+
+ template <typename CharT>
+ class basic_string {
+ public:
+ basic_string();
+ basic_string(const CharT *);
+ basic_string(basic_string_view<CharT>);
+
+ basic_string operator+(const basic_string &) const;
+ };
+ using string = basic_string<char>;
+ using wstring = basic_string<wchar_t>;
+ using u16string = basic_string<char16_t>;
+}
+
+// ==========================================================
+// Positive tests
+// ==========================================================
+
+std::string simpleLiteral() {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+ return "simpleLiteral";
+}
+
+std::string implicitView(std::string_view sv) {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+ return sv;
+}
+
+std::string emptyReturn() {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+ return {};
+}
+
+std::string switchCaseTest(int i) {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+ switch (i) {
+ case 1:
+ return "case1";
+ case 2:
+ return "case2";
+ case 3:
+ return {};
+ default:
+ return "default";
+ }
+}
+
+std::string ifElseTest(bool flag) {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+ if (flag)
+ return "true";
+ return "false";
+}
+
+std::wstring wideStringTest() {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::wstring_view{{.*}}
+ return L"wide literal";
+}
+
+std::u16string u16StringTest() {
+// CHECK-MESSAGES:[[@LINE-1]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::u16string_view{{.*}}
+ return u"u16 literal";
+}
+
+class A {
+ std::string classMethodInt() { return "internal"; }
+// CHECK-MESSAGES:[[@LINE-1]]:3: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+
+ std::string classMethodExt();
+// CHECK-MESSAGES:[[@LINE+4]]:1: {{.*}}[performance-replace-with-string-view]{{.*}}
+// CHECK-FIXES: std::string_view{{.*}}
+};
+
+std::string A::classMethodExt() { return "external"; }
+// CHECK-FIXES: std::string_view{{.*}}
+
+// ==========================================================
+// Negative tests
+// ==========================================================
+
+std::string localVariable() {
+ std::string s = "local variable";
+ return s;
+}
+
+std::string dynamicCalculation() {
+ std::string s1 = "hello ";
+ return s1 + "world";
+}
+
+std::string mixedReturns(bool flag) {
+ if (flag) {
+ return "safe static literal";
+ }
+ std::string s = "unsafe dynamic";
+ return s;
+}
+
+std::string_view alreadyGood() {
+ return "alreadyGood";
+}
+
+std::string returnArgCopy(std::string s) {
+ return s;
+}
+
+std::string returnIndirection(const char* ptr) {
+ // TODO: should be convertible to std::string_view
+ return ptr;
+}
+
+std::string passStringView(std::string_view sv) {
+ // TODO: should be convertible to std::string_view
+ return std::string(sv);
+}
+
+std::string explicitConstruction() {
+ // Cannot be std::string_view: returning address of local temporary object
+ return std::string("explicitConstruction");
+}
+
|
You can test this locally with the following command:git diff -U0 origin/main...HEAD -- clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.cpp clang-tools-extra/clang-tidy/performance/ReplaceWithStringViewCheck.h clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp |
python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py -path build -p1 -quietView the output from clang-tidy here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looks for functions returning
std::[w|u8|u16|u32]stringand suggests to change it tostd::[...]string_viewif possible and profitable.Example:
std::string foo(int i) { // <---- can be replaced to
std::string_view f(...) {switch(i) {
case 1:
return "case1";
case 2:
return "case2";
default:
return {};
}
}