https://github.com/gaul created https://github.com/llvm/llvm-project/pull/209657
Finds `s = s.substr(...)` self-assignments that materialize an unnecessary temporary string and rewrites the prefix-stripping forms (`substr(pos)`, `substr(pos, npos)`) to an in-place `s.erase(0, pos)`. The truncation form `s = s.substr(0, count)` is diagnosed without a fix-it because `s.erase(count)` throws std::out_of_range where `substr` clamps. >From a202934ea931c41bb45c0b34f9b5051f8a92f03e Mon Sep 17 00:00:00 2001 From: Andrew Gaul <[email protected]> Date: Wed, 25 Mar 2026 23:50:04 -0400 Subject: [PATCH] [clang-tidy] Add performance-substr-self-assignment check Finds `s = s.substr(...)` self-assignments that materialize an unnecessary temporary string and rewrites the prefix-stripping forms (`substr(pos)`, `substr(pos, npos)`) to an in-place `s.erase(0, pos)`. The truncation form `s = s.substr(0, count)` is diagnosed without a fix-it because `s.erase(count)` throws std::out_of_range where `substr` clamps. --- .../clang-tidy/performance/CMakeLists.txt | 1 + .../performance/PerformanceTidyModule.cpp | 3 + .../performance/SubstrSelfAssignmentCheck.cpp | 128 +++++++++++++++ .../performance/SubstrSelfAssignmentCheck.h | 44 +++++ clang-tools-extra/docs/ReleaseNotes.rst | 7 + .../docs/clang-tidy/checks/list.rst | 1 + .../performance/substr-self-assignment.rst | 45 +++++ .../checkers/Inputs/Headers/std/string | 1 + .../performance/substr-self-assignment.cpp | 154 ++++++++++++++++++ 9 files changed, 384 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/substr-self-assignment.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/substr-self-assignment.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 0c778b5a9f36b..6851daca87028 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -22,6 +22,7 @@ add_clang_library(clangTidyPerformanceModule STATIC PerformanceTidyModule.cpp PreferSingleCharOverloadsCheck.cpp StringViewConversionsCheck.cpp + SubstrSelfAssignmentCheck.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 a4c1cdacab496..558fd56f19777 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -24,6 +24,7 @@ #include "NoexceptSwapCheck.h" #include "PreferSingleCharOverloadsCheck.h" #include "StringViewConversionsCheck.h" +#include "SubstrSelfAssignmentCheck.h" #include "TriviallyDestructibleCheck.h" #include "TypePromotionInMathFnCheck.h" #include "UnnecessaryCopyInitializationCheck.h" @@ -68,6 +69,8 @@ class PerformanceModule : public ClangTidyModule { "performance-prefer-single-char-overloads"); CheckFactories.registerCheck<StringViewConversionsCheck>( "performance-string-view-conversions"); + CheckFactories.registerCheck<SubstrSelfAssignmentCheck>( + "performance-substr-self-assignment"); CheckFactories.registerCheck<TriviallyDestructibleCheck>( "performance-trivially-destructible"); CheckFactories.registerCheck<TypePromotionInMathFnCheck>( diff --git a/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.cpp b/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.cpp new file mode 100644 index 0000000000000..4d1b9cd4da149 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.cpp @@ -0,0 +1,128 @@ +//===----------------------------------------------------------------------===// +// +// 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 "SubstrSelfAssignmentCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/ExprCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include <optional> +#include <string> + +using namespace clang::ast_matchers; + +namespace clang::tidy::performance { + +// Returns true if E refers to the static member 'npos' of Class. A bare name +// comparison is not enough: a local variable or parameter that happens to be +// called 'npos' carries a real count, which has no single-'erase' rewrite. +static bool isNposOfClass(const Expr *E, const CXXRecordDecl *Class) { + const ValueDecl *D = nullptr; + E = E->IgnoreParenImpCasts(); + if (const auto *Ref = dyn_cast<DeclRefExpr>(E)) + D = Ref->getDecl(); + else if (const auto *Member = dyn_cast<MemberExpr>(E)) + D = Member->getMemberDecl(); + const auto *Var = dyn_cast_or_null<VarDecl>(D); + if (!Var || !Var->isStaticDataMember() || Var->getName() != "npos") + return false; + return declaresSameEntity(dyn_cast<CXXRecordDecl>(Var->getDeclContext()), + Class); +} + +SubstrSelfAssignmentCheck::SubstrSelfAssignmentCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + StringLikeClasses(utils::options::parseStringList( + Options.get("StringLikeClasses", "::std::basic_string"))) {} + +void SubstrSelfAssignmentCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StringLikeClasses", + utils::options::serializeStringList(StringLikeClasses)); +} + +void SubstrSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { + const auto VarRef = + ignoringParens(declRefExpr(to(varDecl().bind("var"))).bind("lhs")); + + const auto SubstrCall = + cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("substr"), + ofClass(hasAnyName(StringLikeClasses)))), + on(ignoringParens(declRefExpr(to(varDecl(equalsBoundNode("var"))))))) + .bind("substr"); + + // Match: s = s.substr(...) + Finder->addMatcher(cxxOperatorCallExpr(hasOperatorName("="), + hasArgument(0, VarRef), + hasArgument(1, SubstrCall)) + .bind("assign"), + this); +} + +void SubstrSelfAssignmentCheck::check(const MatchFinder::MatchResult &Result) { + const auto *AssignExpr = + Result.Nodes.getNodeAs<CXXOperatorCallExpr>("assign"); + const auto *LHS = Result.Nodes.getNodeAs<DeclRefExpr>("lhs"); + const auto *SubstrExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("substr"); + const SourceManager &SM = *Result.SourceManager; + const LangOptions &LangOpts = Result.Context->getLangOpts(); + + const CXXMethodDecl *Method = SubstrExpr->getMethodDecl(); + if (!Method) + return; + + // Count only explicitly-written arguments (exclude CXXDefaultArgExpr). + unsigned NumExplicitArgs = 0; + for (const Expr *Arg : SubstrExpr->arguments()) + if (!isa<CXXDefaultArgExpr>(Arg)) + ++NumExplicitArgs; + + // s = s.substr(pos) and s = s.substr(pos, npos) strip a prefix and can be + // rewritten as s.erase(0, pos). The truncation form s = s.substr(0, count) + // is diagnosed without a fix-it: s.erase(count) throws std::out_of_range + // for count > s.size() where substr merely clamps. The general + // two-argument form has no single-'erase' equivalent and is ignored. + const bool IsPrefixStrip = + NumExplicitArgs == 1 || + (NumExplicitArgs == 2 && + isNposOfClass(SubstrExpr->getArg(1), Method->getParent())); + bool IsTruncation = false; + if (!IsPrefixStrip && NumExplicitArgs == 2) { + const auto *PosLiteral = + dyn_cast<IntegerLiteral>(SubstrExpr->getArg(0)->IgnoreParenImpCasts()); + IsTruncation = PosLiteral && PosLiteral->getValue() == 0; + } + if (!IsPrefixStrip && !IsTruncation) + return; + + // Rewriting a macro expansion is unsafe; emit the warning without a fix-it. + std::optional<std::string> Replacement; + if (IsPrefixStrip && !AssignExpr->getBeginLoc().isMacroID() && + !AssignExpr->getEndLoc().isMacroID()) { + StringRef VarName = Lexer::getSourceText( + CharSourceRange::getTokenRange(LHS->getSourceRange()), SM, LangOpts); + StringRef PosText = Lexer::getSourceText( + CharSourceRange::getTokenRange(SubstrExpr->getArg(0)->getSourceRange()), + SM, LangOpts); + if (!VarName.empty() && !PosText.empty()) + Replacement = (VarName + ".erase(0, " + PosText + ")").str(); + } + + auto Diag = diag(AssignExpr->getOperatorLoc(), + "inefficient self-assignment via 'substr'; use 'erase' to " + "modify the string in-place"); + if (Replacement) + Diag << FixItHint::CreateReplacement(AssignExpr->getSourceRange(), + *Replacement); +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.h b/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.h new file mode 100644 index 0000000000000..cf9dc425c44d2 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/SubstrSelfAssignmentCheck.h @@ -0,0 +1,44 @@ +//===----------------------------------------------------------------------===// +// +// 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_SUBSTRSELFASSIGNMENTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_SUBSTRSELFASSIGNMENTCHECK_H + +#include "../ClangTidyCheck.h" + +#include <vector> + +namespace clang::tidy::performance { + +/// Finds cases where a string variable is assigned the result of calling +/// ``substr()`` on itself (e.g., ``s = s.substr(x, y)``). This pattern creates +/// an unnecessary temporary string; the same effect can be achieved in-place +/// using ``erase()``. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/performance/substr-self-assignment.html +class SubstrSelfAssignmentCheck : public ClangTidyCheck { +public: + SubstrSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + const std::vector<StringRef> StringLikeClasses; +}; + +} // namespace clang::tidy::performance + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_SUBSTRSELFASSIGNMENTCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 69c3bcf67b8db..6dbcf2eb48b4c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,13 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`performance-substr-self-assignment + <clang-tidy/checks/performance/substr-self-assignment>` check. + + Finds cases where a string variable is assigned the result of calling + ``substr()`` on itself, which creates an unnecessary temporary. Suggests + using ``erase()`` to modify the string in-place instead. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2a44dc78fbc89..54e2b0d69b2e5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -367,6 +367,7 @@ Clang-Tidy Checks :doc:`performance-noexcept-swap <performance/noexcept-swap>`, "Yes" :doc:`performance-prefer-single-char-overloads <performance/prefer-single-char-overloads>`, "Yes" :doc:`performance-string-view-conversions <performance/string-view-conversions>`, "Yes" + :doc:`performance-substr-self-assignment <performance/substr-self-assignment>`, "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/substr-self-assignment.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/substr-self-assignment.rst new file mode 100644 index 0000000000000..d71030dc15ae1 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/substr-self-assignment.rst @@ -0,0 +1,45 @@ +.. title:: clang-tidy - performance-substr-self-assignment + +performance-substr-self-assignment +=================================== + +Finds cases where a string variable is assigned the result of calling +``substr()`` on itself. This pattern materializes an unnecessary temporary +string (an allocation, a copy of the surviving characters, and a +deallocation) and discards the original capacity; the same effect can be +achieved in-place with ``erase()``. + +.. code-block:: c++ + + std::string s = "hello world"; + + s = s.substr(5); // warning; fix-it: s.erase(0, 5) + s = s.substr(5, std::string::npos); // warning; fix-it: s.erase(0, 5) + s = s.substr(0, 3); // warning; no fix-it (see below) + +The fix-it for the prefix-stripping forms replaces the assignment with +``s.erase(0, pos)``. The two expressions differ only when +``pos > s.size()``: ``substr`` throws ``std::out_of_range``, while +``erase(0, pos)`` clamps and erases the whole string. Code that relies on +that exception changes behavior under the fix-it. + +The truncation form ``s = s.substr(0, count)`` is diagnosed without a +fix-it: the tempting replacement ``s.erase(count)`` throws +``std::out_of_range`` whenever ``count > s.size()``, whereas ``substr`` +clamps ``count`` and leaves the string unchanged. A safe manual rewrite is +``s.resize(std::min(count, s.size()))``. + +No diagnostic is emitted for the general form ``s = s.substr(pos, count)``, +which has no single-call in-place equivalent. Inside macro expansions the +warning is emitted without a fix-it. Only self-assignments to plain +variables are diagnosed; assignments through class members or pointers are +not. + +Options +------- + +.. option:: StringLikeClasses + + Semicolon-separated list of names of string-like classes. By default only + ``::std::basic_string`` is considered. Classes listed here must provide + ``substr``, ``erase``, and ``npos`` with ``std::basic_string`` semantics. diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string index dbebeaaa46514..edb7dc587d865 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string @@ -75,6 +75,7 @@ struct basic_string { _Type& insert(size_type pos, const C* s); _Type& insert(size_type pos, const C* s, size_type n); + _Type& erase(size_type pos = 0, size_type count = npos); _Type substr(size_type pos = 0, size_type count = npos) const; constexpr bool starts_with(std::basic_string_view<C, T> sv) const noexcept; diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/substr-self-assignment.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/substr-self-assignment.cpp new file mode 100644 index 0000000000000..0ad421ffd98f3 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/substr-self-assignment.cpp @@ -0,0 +1,154 @@ +// RUN: %check_clang_tidy %s performance-substr-self-assignment %t +#include <string> + +void OneArg() { + std::string s = "hello world"; + + // Basic case: s = s.substr(pos) + s = s.substr(5); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr'; use 'erase' to modify the string in-place [performance-substr-self-assignment] + // CHECK-FIXES: s.erase(0, 5); + + // With a variable as the argument. + int pos = 3; + s = s.substr(pos); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, pos); + + // With a more complex expression. + s = s.substr(pos + 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, pos + 1); +} + +void TruncationNoFixIt() { + std::string s = "hello world"; + + // The truncation form s = s.substr(0, count) is diagnosed but gets no + // fix-it: 'erase(count)' throws std::out_of_range when count > size(), + // while 'substr(0, count)' clamps. + s = s.substr(0, 5); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr'; use 'erase' to modify the string in-place [performance-substr-self-assignment] + // CHECK-FIXES: s = s.substr(0, 5); + + size_t count = 3; + s = s.substr(0, count); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s = s.substr(0, count); +} + +void TwoArgWithNpos() { + std::string s = "hello world"; + + // s = s.substr(pos, npos) is equivalent to s = s.substr(pos). + s = s.substr(3, std::string::npos); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr'; use 'erase' to modify the string in-place [performance-substr-self-assignment] + // CHECK-FIXES: s.erase(0, 3); + + int pos = 2; + s = s.substr(pos, std::string::npos); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, pos); + + // Member-syntax spelling of npos. + s = s.substr(4, s.npos); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, 4); +} + +void LocalVariableNamedNpos() { + std::string s = "hello world"; + + // A local variable named 'npos' is not std::basic_string::npos; the count + // is meaningful, so there is no single-'erase' rewrite and no warning. + size_t npos = 3; + s = s.substr(2, npos); +} + +void ParamNamedNpos(std::string s, std::string::size_type npos) { + s = s.substr(1, npos); +} + +void TwoArgGeneral() { + std::string s = "hello world"; + + // General two-argument case: no diagnostic (no single erase() equivalent). + s = s.substr(2, 3); + + int pos = 1; + size_t len = 4; + s = s.substr(pos, len); +} + +void Negatives() { + std::string s = "hello"; + std::string t = "world"; + + // Different variables -- not a self-assignment. + s = t.substr(1); + s = t.substr(0, 3); + + // Not an assignment to the same variable. + std::string r = s.substr(1); +} + +struct Holder { + std::string Str; + // Self-assignment through a class member is not matched; only plain + // variables are. + void trim() { Str = Str.substr(2); } +}; + +template <typename T> +void dependentType(T t) { + // Type-dependent: no diagnostic, including in instantiations. + t = t.substr(1); +} +void instantiate() { dependentType(std::string("hello")); } + +void WideString() { + std::wstring ws = L"hello world"; + + ws = ws.substr(3); + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: ws.erase(0, 3); + + // Truncation form: warning only, no fix-it. + ws = ws.substr(0, 5); + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: ws = ws.substr(0, 5); +} + +void Parenthesized() { + std::string s = "hello"; + + // Parenthesized object on the RHS -- should still match. + s = (s).substr(2); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, 2); + + // Parenthesized substr call on the RHS. + s = (s.substr(3)); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, 3); +} + +#define STRIP_PREFIX(str, n) str = str.substr(n) +void MacroExpansion() { + std::string s = "hello"; + + // Diagnosed, but no fix-it: rewriting a macro expansion is unsafe. + STRIP_PREFIX(s, 2); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: STRIP_PREFIX(s, 2); +} + +#define OFFSET 2 +void MacroArgument() { + std::string s = "hello"; + + // Only the argument comes from a macro; the fix-it preserves its spelling. + s = s.substr(OFFSET); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient self-assignment via 'substr' + // CHECK-FIXES: s.erase(0, OFFSET); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
