-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[clang-tidy] Add a new check 'replace-with-string-view' #172170
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||
| //===----------------------------------------------------------------------===// | ||||||
| // | ||||||
| // 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 { | ||||||
|
|
||||||
| static llvm::StringLiteral toStringViewTypeStr(StringRef Type) { | ||||||
| if (Type.contains("wchar_t")) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does type can have anything other than just |
||||||
| 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"; | ||||||
|
Comment on lines
+19
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add tests with these types?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well... not now. Seems there're some problems with u/U/L prefixes support in our mock-headers.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you adjust those headers to properly handle prefixes?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add tests with u8 before merging. |
||||||
| return "std::string_view"; | ||||||
| } | ||||||
|
|
||||||
| void ReplaceWithStringViewCheck::registerMatchers(MatchFinder *Finder) { | ||||||
| const auto IsStdString = hasCanonicalType( | ||||||
| hasDeclaration(cxxRecordDecl(hasName("::std::basic_string")))); | ||||||
| const auto IsStdStringView = expr(hasType(hasCanonicalType( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we more clearly apply
Suggested change
|
||||||
| hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view")))))); | ||||||
|
|
||||||
| Finder->addMatcher( | ||||||
| functionDecl( | ||||||
| isDefinition(), unless(cxxMethodDecl(isVirtual())), | ||||||
| returns(IsStdString), hasDescendant(returnStmt()), | ||||||
irishrover marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| 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) { | ||||||
| const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("func"); | ||||||
| assert(MatchedDecl); | ||||||
| const llvm::StringLiteral DestReturnTypeStr = toStringViewTypeStr( | ||||||
| MatchedDecl->getReturnType().getCanonicalType().getAsString()); | ||||||
|
|
||||||
| auto Diag = diag(MatchedDecl->getReturnTypeSourceRange().getBegin(), | ||||||
| "consider using '%0' to avoid unnecessary " | ||||||
| "copying and allocations") | ||||||
| << DestReturnTypeStr; | ||||||
|
|
||||||
| for (const auto *FuncDecl : MatchedDecl->redecls()) | ||||||
| Diag << FixItHint::CreateReplacement(FuncDecl->getReturnTypeSourceRange(), | ||||||
| DestReturnTypeStr); | ||||||
| } | ||||||
|
|
||||||
| } // namespace clang::tidy::performance | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // 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. | ||
| /// | ||
| /// 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| .. 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 if possible. | ||
|
|
||
| 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 sometimes be eliminated 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 their return type to ``std::string_view``. | ||
|
|
||
| It handles ``std::string``, ``std::wstring``, ``std::u8string``, | ||
| ``std::u16string`` and ``std::u32string`` along with their aliases and selects | ||
| the proper kind of ``std::string_view`` to return. | ||
|
|
||
| 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. |
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.
I'm not sure if we should use
StringRef.llvm-project/llvm/include/llvm/ADT/StringRef.h
Lines 849 to 850 in f2d48dd
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.
@vbvictor, what do you think of
misc-*?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.
You mean StringRef or StringLiteral?
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.
I think we typically use
StringRefin such cases just likestring_view.There are far less StringLiterals in clang-tidy usage (136 vs 2100)