https://github.com/gaul created https://github.com/llvm/llvm-project/pull/219880
Finds assignments and appends of a `substr()` result to another string (`dst = src.substr(p, n)`, `s += t.substr(p, n)`) and rewrites them to the `assign()`/`append()` overloads taking a string, position, and count, which avoid materializing a temporary string. The rewrite is exception-exact: `substr(p, n)` and `assign/append(str, p, n)` throw std::out_of_range under the same condition and clamp the count identically, so the fix-its preserve behavior for every argument value. Self-assignment `s = s.substr(p)` is not diagnosed: the `assign` rewrite would self-alias, and an in-place `erase` of the removed prefix is strictly better; `s += s.substr(p)` is diagnosed without a fix-it to avoid introducing a self-aliasing call. The check fires on real code: about 20 sites across llvm-project (e.g. clang/lib/Format/Format.cpp, llvm/tools/llvm-cov/SourceCoverageView.cpp), while correctly ignoring the far more common llvm::StringRef::substr, which is a free view slice. References #209657. >From 7a89f9df4bca06cc2d53225a15f22d96292223a0 Mon Sep 17 00:00:00 2001 From: Andrew Gaul <[email protected]> Date: Tue, 14 Jul 2026 19:05:27 -0700 Subject: [PATCH] [clang-tidy] Add performance-inefficient-substr check Finds assignments and appends of a `substr()` result to another string (`dst = src.substr(p, n)`, `s += t.substr(p, n)`) and rewrites them to the `assign()`/`append()` overloads taking a string, position, and count, which avoid materializing a temporary string. The rewrite is exception-exact: `substr(p, n)` and `assign/append(str, p, n)` throw std::out_of_range under the same condition and clamp the count identically, so the fix-its preserve behavior for every argument value. Self-assignment `s = s.substr(p)` is not diagnosed: the `assign` rewrite would self-alias, and an in-place `erase` of the removed prefix is strictly better; `s += s.substr(p)` is diagnosed without a fix-it to avoid introducing a self-aliasing call. The check fires on real code: about 20 sites across llvm-project (e.g. clang/lib/Format/Format.cpp, llvm/tools/llvm-cov/SourceCoverageView.cpp), while correctly ignoring the far more common llvm::StringRef::substr, which is a free view slice. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RN4gpjgX8QWXRuQoBzYace --- .../clang-tidy/performance/CMakeLists.txt | 1 + .../performance/InefficientSubstrCheck.cpp | 123 +++++++++++++ .../performance/InefficientSubstrCheck.h | 44 +++++ .../performance/PerformanceTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.md | 7 + .../docs/clang-tidy/checks/list.md | 1 + .../checks/performance/inefficient-substr.md | 43 +++++ .../checkers/Inputs/Headers/std/string | 2 + .../performance/inefficient-substr.cpp | 163 ++++++++++++++++++ 9 files changed, 387 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-substr.md create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-substr.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index f55a6cf2800f3..7c022662238a3 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyPerformanceModule STATIC ImplicitConversionInLoopCheck.cpp InefficientAlgorithmCheck.cpp InefficientStringConcatenationCheck.cpp + InefficientSubstrCheck.cpp InefficientVectorOperationCheck.cpp MoveConstArgCheck.cpp MoveConstructorInitCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.cpp new file mode 100644 index 0000000000000..0d1a977462503 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.cpp @@ -0,0 +1,123 @@ +//===----------------------------------------------------------------------===// +// +// 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 "InefficientSubstrCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.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 { + +InefficientSubstrCheck::InefficientSubstrCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + StringLikeClasses(utils::options::parseStringList( + Options.get("StringLikeClasses", "::std::basic_string"))) {} + +void InefficientSubstrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StringLikeClasses", + utils::options::serializeStringList(StringLikeClasses)); +} + +void InefficientSubstrCheck::registerMatchers(MatchFinder *Finder) { + const auto LhsRef = + ignoringParens(declRefExpr(to(varDecl().bind("lhs-var"))).bind("lhs")); + + const auto SubstrCall = + cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("substr"), + ofClass(hasAnyName(StringLikeClasses)))), + on(ignoringParens( + declRefExpr(to(varDecl().bind("src-var"))).bind("src")))) + .bind("substr"); + + // Match: lhs = src.substr(...) and lhs += src.substr(...) + Finder->addMatcher(cxxOperatorCallExpr(hasAnyOperatorName("=", "+="), + hasArgument(0, LhsRef), + hasArgument(1, SubstrCall)) + .bind("op"), + this); +} + +void InefficientSubstrCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("op"); + const auto *LHS = Result.Nodes.getNodeAs<DeclRefExpr>("lhs"); + const auto *Src = Result.Nodes.getNodeAs<DeclRefExpr>("src"); + const auto *LHSVar = Result.Nodes.getNodeAs<VarDecl>("lhs-var"); + const auto *SrcVar = Result.Nodes.getNodeAs<VarDecl>("src-var"); + const auto *SubstrExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("substr"); + const SourceManager &SM = *Result.SourceManager; + const LangOptions &LangOpts = Result.Context->getLangOpts(); + + const bool IsAppend = Op->getOperator() == OO_PlusEqual; + const bool SameVar = declaresSameEntity(LHSVar, SrcVar); + + // s = s.substr(...) is excluded: the 'assign' rewrite would self-alias, + // and the strictly better rewrite is an in-place 'erase' of the prefix. + if (!IsAppend && SameVar) + return; + + // The (str, pos, count) overload must belong to the same basic_string + // specialization as the destination so that it applies without + // conversions. + if (!ASTContext::hasSameUnqualifiedType(LHS->getType(), Src->getType())) + return; + + // Count only explicitly-written arguments (exclude CXXDefaultArgExpr). + // s += t.substr() is just s += t in disguise; leave it alone. + SmallVector<const Expr *, 2> ExplicitArgs; + for (const Expr *Arg : SubstrExpr->arguments()) + if (!isa<CXXDefaultArgExpr>(Arg)) + ExplicitArgs.push_back(Arg); + if (ExplicitArgs.empty()) + return; + + // substr(pos, count) and assign/append(str, pos, count) throw and clamp + // identically, so the arguments pass through verbatim. Self-appends + // (s += s.substr(...)) are not rewritten because the replacement would + // introduce a self-aliasing call, and macro expansions are not rewritten + // because editing them is unsafe; both still get the warning. + std::optional<std::string> Replacement; + if (!SameVar && !Op->getBeginLoc().isMacroID() && + !Op->getEndLoc().isMacroID()) { + const auto GetText = [&](SourceRange R) { + return Lexer::getSourceText(CharSourceRange::getTokenRange(R), SM, + LangOpts); + }; + StringRef LHSText = GetText(LHS->getSourceRange()); + StringRef SrcText = GetText(Src->getSourceRange()); + bool Valid = !LHSText.empty() && !SrcText.empty(); + std::string Args; + for (const Expr *Arg : ExplicitArgs) { + StringRef ArgText = GetText(Arg->getSourceRange()); + Valid = Valid && !ArgText.empty(); + Args += ", "; + Args += ArgText; + } + if (Valid) + Replacement = (LHSText + (IsAppend ? ".append(" : ".assign(") + SrcText + + Args + ")") + .str(); + } + + auto Diag = diag(Op->getOperatorLoc(), + "inefficient %select{assignment|concatenation}0 via " + "'substr' temporary; use '%select{assign|append}0' to " + "avoid the temporary string") + << (IsAppend ? 1 : 0); + if (Replacement) + Diag << FixItHint::CreateReplacement(Op->getSourceRange(), *Replacement); +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.h new file mode 100644 index 0000000000000..29af80dba0f78 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientSubstrCheck.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_INEFFICIENTSUBSTRCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTSUBSTRCHECK_H + +#include "../ClangTidyCheck.h" + +#include <vector> + +namespace clang::tidy::performance { + +/// Finds assignments and appends of a ``substr()`` result to another string +/// (e.g., ``dst = src.substr(pos)`` or ``s += t.substr(pos, count)``) and +/// suggests the ``assign()``/``append()`` overloads taking a string, +/// position, and count, which avoid materializing a temporary string. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-substr.html +class InefficientSubstrCheck : public ClangTidyCheck { +public: + InefficientSubstrCheck(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_INEFFICIENTSUBSTRCHECK_H diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 9eee02494be91..ecbe39718fe8e 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -15,6 +15,7 @@ #include "ImplicitConversionInLoopCheck.h" #include "InefficientAlgorithmCheck.h" #include "InefficientStringConcatenationCheck.h" +#include "InefficientSubstrCheck.h" #include "InefficientVectorOperationCheck.h" #include "MoveConstArgCheck.h" #include "MoveConstructorInitCheck.h" @@ -52,6 +53,8 @@ class PerformanceModule : public ClangTidyModule { "performance-inefficient-algorithm"); CheckFactories.registerCheck<InefficientStringConcatenationCheck>( "performance-inefficient-string-concatenation"); + CheckFactories.registerCheck<InefficientSubstrCheck>( + "performance-inefficient-substr"); CheckFactories.registerCheck<InefficientVectorOperationCheck>( "performance-inefficient-vector-operation"); CheckFactories.registerCheck<MoveConstArgCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 633418a2abb98..d0b83daf4aa2d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -113,6 +113,13 @@ infrastructure are described first, followed by tool-specific sections. Finds calls to `value_or` (and alternative spellings `valueOr`, `ValueOr`) on optional types where the return type is expensive to copy. +- New {doc}`performance-inefficient-substr + <clang-tidy/checks/performance/inefficient-substr>` check. + + Finds assignments and appends of a `substr()` result to another string + and suggests the `assign()`/`append()` overloads taking a string, + position, and count, which avoid the temporary string. + - New {doc}`portability-avoid-pragma-comment <clang-tidy/checks/portability/avoid-pragma-comment>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.md b/clang-tools-extra/docs/clang-tidy/checks/list.md index 5a220b13eb599..b0e0bfd16cb4f 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.md +++ b/clang-tools-extra/docs/clang-tidy/checks/list.md @@ -358,6 +358,7 @@ readability/* | {doc}`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>` | | | {doc}`performance-inefficient-algorithm <performance/inefficient-algorithm>` | Yes | | {doc}`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>` | | +| {doc}`performance-inefficient-substr <performance/inefficient-substr>` | Yes | | {doc}`performance-inefficient-vector-operation <performance/inefficient-vector-operation>` | Yes | | {doc}`performance-move-const-arg <performance/move-const-arg>` | Yes | | {doc}`performance-move-constructor-init <performance/move-constructor-init>` | | diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-substr.md b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-substr.md new file mode 100644 index 0000000000000..a970d5dfb958f --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-substr.md @@ -0,0 +1,43 @@ +```{title} clang-tidy - performance-inefficient-substr +``` + +# performance-inefficient-substr + +Finds assignments and appends of a `substr()` result to another string and +suggests the `assign()`/`append()` overloads taking a string, position, +and count, which avoid materializing a temporary string (an allocation, a +copy of the surviving characters, and a deallocation). + +```cpp +std::string dst, s; +std::string src = "hello world"; + +dst = src.substr(6); // fix-it: dst.assign(src, 6) +s += src.substr(0, 5); // fix-it: s.append(src, 0, 5) +``` + +The rewrite is exact: `substr(pos, count)`, `assign(str, pos, count)`, and +`append(str, pos, count)` all throw `std::out_of_range` if and only if +`pos > str.size()` and clamp `count` the same way, so the fix-it preserves +behavior for every argument value, including `npos` counts. + +`s += s.substr(pos)` is diagnosed but not rewritten: the replacement would +introduce a self-aliasing `append` call, which the check conservatively +avoids. `s = s.substr(pos)` is not diagnosed by this check at all: the +`assign` rewrite would self-alias too, and the strictly better rewrite for +that case is an in-place `erase` of the removed prefix. + +Inside macro expansions the warning is emitted without a fix-it. Only plain +variables are matched on both sides; class members and pointers are not. +Initializations such as `std::string t = s.substr(1);` are not diagnosed: +guaranteed copy elision already makes them cheap. + +## 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` and the `(string, position, count)` overloads of `assign` and +`append` 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 766f240c655fb..621722539cafc 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 @@ -42,8 +42,10 @@ struct basic_string { _Type& append(const C *s); _Type& append(const C *s, size_type n); + _Type& append(const _Type& str, size_type pos, size_type count = npos); _Type& assign(const C *s); _Type& assign(const C *s, size_type n); + _Type& assign(const _Type& str, size_type pos, size_type count = npos); int compare(const _Type&) const; int compare(const C* s) const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-substr.cpp new file mode 100644 index 0000000000000..ee176c3bd1654 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-substr.cpp @@ -0,0 +1,163 @@ +// RUN: %check_clang_tidy %s performance-inefficient-substr %t +#include <string> + +void AppendForm() { + std::string s = "hello"; + std::string t = "world wide"; + + s += t.substr(5); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' temporary; use 'append' to avoid the temporary string [performance-inefficient-substr] + // CHECK-FIXES: s.append(t, 5); + + s += t.substr(5, 3); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, 5, 3); + + // Expression arguments pass through verbatim. + int pos = 1; + int len = 4; + s += t.substr(pos + 1, len - 2); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, pos + 1, len - 2); + + // npos passes through verbatim: append clamps it identically. + s += t.substr(2, std::string::npos); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, 2, std::string::npos); + + const std::string c = "const source"; + s += c.substr(3); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(c, 3); + + // Parenthesized receiver and parenthesized call. + s += (t).substr(2); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, 2); + + s += (t.substr(2)); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, 2); +} + +void AssignForm() { + std::string dst; + std::string src = "hello world"; + + dst = src.substr(2); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: inefficient assignment via 'substr' temporary; use 'assign' to avoid the temporary string [performance-inefficient-substr] + // CHECK-FIXES: dst.assign(src, 2); + + dst = src.substr(2, 3); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: inefficient assignment via 'substr' + // CHECK-FIXES: dst.assign(src, 2, 3); +} + +void SelfAssign() { + std::string s = "hello"; + + // Same-variable assignment is excluded: an in-place 'erase' rewrite is + // strictly better than a self-aliasing 'assign'; this check stays silent. + s = s.substr(2); +} + +void SelfAppend() { + std::string s = "hello"; + + // Diagnosed, but not rewritten: the replacement would introduce a + // self-aliasing append(s, ...) call. + s += s.substr(3); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s += s.substr(3); +} + +void ZeroArg() { + std::string s = "hello"; + std::string t = "world"; + + // s += t.substr() is just s += t in disguise; no diagnostic. + s += t.substr(); +} + +struct MyString { + MyString substr(unsigned pos) const; + MyString &operator+=(const MyString &); + MyString &operator=(const MyString &); +}; + +void NotStringLike(MyString a, MyString b) { + // Not in StringLikeClasses; no diagnostic. + a += b.substr(1); + a = b.substr(1); +} + +struct Derived : std::string {}; + +void DerivedReceiver(std::string s, Derived d) { + // Receiver type differs from the destination type; the check is + // conservative and stays silent. + s += d.substr(1); +} + +struct Holder { + std::string S; + void add(const std::string &t) { + // Class members are not matched; only plain variables are. + S += t.substr(1); + } +}; + +void MemberSource(Holder h) { + std::string s; + s += h.S.substr(1); +} + +template <typename T> +void dependentType(T a, T b) { + // Type-dependent: no diagnostic, including in instantiations. + a += b.substr(1); +} +void instantiate() { + dependentType(std::string("hello"), std::string("world")); +} + +void Initialization(std::string s) { + // Initializations are copy-elided since C++17; nothing to save. + std::string t = s.substr(1); + std::string u(s.substr(1)); +} + +void WideString() { + std::wstring wd; + std::wstring ws = L"hello world"; + + wd += ws.substr(1); + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: wd.append(ws, 1); + + wd = ws.substr(1); + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: inefficient assignment via 'substr' + // CHECK-FIXES: wd.assign(ws, 1); +} + +#define APPEND_TAIL(a, b, n) a += b.substr(n) +void MacroExpansion() { + std::string s = "hello"; + std::string t = "world"; + + // Diagnosed, but no fix-it: rewriting a macro expansion is unsafe. + APPEND_TAIL(s, t, 2); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: APPEND_TAIL(s, t, 2); +} + +#define OFFSET 2 +void MacroArgument() { + std::string s = "hello"; + std::string t = "world"; + + // Only the argument comes from a macro; the fix-it preserves its spelling. + s += t.substr(OFFSET); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: inefficient concatenation via 'substr' + // CHECK-FIXES: s.append(t, OFFSET); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
