[clang-tools-extra] [clang-tidy] Add readability-string-view-substr check (PR #120055)
Helmut Januschka via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 7 14:55:06 PST 2025
https://github.com/hjanuschka updated https://github.com/llvm/llvm-project/pull/120055
>From 8b2dc9adf4fae2065823e5beb3a1cd851686913c Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Mon, 16 Dec 2024 08:24:14 +0100
Subject: [PATCH 1/9] [clang-tidy] Add readability-string-view-substr check
Add a new check that suggests using string_view::remove_prefix() and
remove_suffix() instead of substr() when the intent is to remove
characters from either end of a string_view.
---
.../clang-tidy/readability/CMakeLists.txt | 1 +
.../readability/ReadabilityTidyModule.cpp | 3 +
.../readability/StringViewSubstrCheck.cpp | 132 ++++++++++++++++++
.../readability/StringViewSubstrCheck.h | 39 ++++++
clang-tools-extra/docs/ReleaseNotes.rst | 7 +
.../checks/readability/string-view-substr.rst | 16 +++
.../readability/stringview_substr.cpp | 55 ++++++++
7 files changed, 253 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 8f303c51e1b0da..8b44fc339441ac 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -53,6 +53,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
StaticAccessedThroughInstanceCheck.cpp
StaticDefinitionInAnonymousNamespaceCheck.cpp
StringCompareCheck.cpp
+ StringViewSubstrCheck.cpp
SuspiciousCallArgumentCheck.cpp
UniqueptrDeleteReleaseCheck.cpp
UppercaseLiteralSuffixCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index d61c0ba39658e5..f36ec8f95ede60 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -56,6 +56,7 @@
#include "StaticAccessedThroughInstanceCheck.h"
#include "StaticDefinitionInAnonymousNamespaceCheck.h"
#include "StringCompareCheck.h"
+#include "StringViewSubstrCheck.h"
#include "SuspiciousCallArgumentCheck.h"
#include "UniqueptrDeleteReleaseCheck.h"
#include "UppercaseLiteralSuffixCheck.h"
@@ -146,6 +147,8 @@ class ReadabilityModule : public ClangTidyModule {
"readability-static-definition-in-anonymous-namespace");
CheckFactories.registerCheck<StringCompareCheck>(
"readability-string-compare");
+ CheckFactories.registerCheck<StringViewSubstrCheck>(
+ "readability-stringview-substr");
CheckFactories.registerCheck<readability::NamedParameterCheck>(
"readability-named-parameter");
CheckFactories.registerCheck<NonConstParameterCheck>(
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
new file mode 100644
index 00000000000000..e86a971695a835
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -0,0 +1,132 @@
+//===--- StringViewSubstrCheck.cpp - clang-tidy------------------*- C++ -*-===//
+//
+// 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 "StringViewSubstrCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+void StringViewSubstrCheck::registerMatchers(MatchFinder *Finder) {
+ const auto HasStringViewType = hasType(hasUnqualifiedDesugaredType(recordType(
+ hasDeclaration(recordDecl(hasName("::std::basic_string_view"))))));
+
+ // Match assignment to string_view's substr
+ Finder->addMatcher(
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="),
+ hasArgument(0, expr(HasStringViewType).bind("target")),
+ hasArgument(
+ 1, cxxMemberCallExpr(callee(memberExpr(hasDeclaration(
+ cxxMethodDecl(hasName("substr"))))),
+ on(expr(HasStringViewType).bind("source")))
+ .bind("substr_call")))
+ .bind("assignment"),
+ this);
+}
+
+void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Assignment =
+ Result.Nodes.getNodeAs<CXXOperatorCallExpr>("assignment");
+ const auto *Target = Result.Nodes.getNodeAs<Expr>("target");
+ const auto *Source = Result.Nodes.getNodeAs<Expr>("source");
+ const auto *SubstrCall =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>("substr_call");
+
+ if (!Assignment || !Target || !Source || !SubstrCall) {
+ return;
+ }
+
+ // Get the DeclRefExpr for the target and source to compare variables
+ const auto *TargetDRE = dyn_cast<DeclRefExpr>(Target->IgnoreParenImpCasts());
+ const auto *SourceDRE = dyn_cast<DeclRefExpr>(Source->IgnoreParenImpCasts());
+
+ // Only handle self-assignment cases
+ if (!TargetDRE || !SourceDRE ||
+ TargetDRE->getDecl() != SourceDRE->getDecl()) {
+ return;
+ }
+
+ const Expr *StartArg = SubstrCall->getArg(0);
+ const Expr *LengthArg =
+ SubstrCall->getNumArgs() > 1 ? SubstrCall->getArg(1) : nullptr;
+
+ // Get source text of first argument
+ std::string StartText =
+ Lexer::getSourceText(
+ CharSourceRange::getTokenRange(StartArg->getSourceRange()),
+ *Result.SourceManager, Result.Context->getLangOpts())
+ .str();
+
+ // Case 1: Check for remove_prefix pattern - only when the second arg is
+ // missing (uses npos)
+ if (!LengthArg || isa<CXXDefaultArgExpr>(LengthArg)) {
+ std::string Replacement = TargetDRE->getNameInfo().getAsString() +
+ ".remove_prefix(" + StartText + ")";
+ diag(Assignment->getBeginLoc(), "prefer 'remove_prefix' over 'substr' for "
+ "removing characters from the start")
+ << FixItHint::CreateReplacement(Assignment->getSourceRange(),
+ Replacement);
+ return;
+ }
+
+ // Case 2: Check for remove_suffix pattern
+ if (StartText == "0") {
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(LengthArg)) {
+ if (BinOp->getOpcode() == BO_Sub) {
+ const Expr *LHS = BinOp->getLHS();
+ const Expr *RHS = BinOp->getRHS();
+
+ // Check if LHS is a length() call on the same string_view
+ if (const auto *LengthCall = dyn_cast<CXXMemberCallExpr>(LHS)) {
+ if (const auto *LengthMethod =
+ dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
+ if (LengthMethod->getName() == "length") {
+ // Verify the length() call is on the same string_view
+ const Expr *LengthObject =
+ LengthCall->getImplicitObjectArgument();
+ const auto *LengthDRE =
+ dyn_cast<DeclRefExpr>(LengthObject->IgnoreParenImpCasts());
+
+ if (!LengthDRE || LengthDRE->getDecl() != TargetDRE->getDecl()) {
+ return;
+ }
+
+ // Must be a simple non-zero integer literal
+ const auto *IL =
+ dyn_cast<IntegerLiteral>(RHS->IgnoreParenImpCasts());
+ if (!IL || IL->getValue() == 0) {
+ return;
+ }
+
+ std::string RHSText =
+ Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RHS->getSourceRange()),
+ *Result.SourceManager, Result.Context->getLangOpts())
+ .str();
+
+ std::string Replacement = TargetDRE->getNameInfo().getAsString() +
+ ".remove_suffix(" + RHSText + ")";
+ diag(Assignment->getBeginLoc(),
+ "prefer 'remove_suffix' over 'substr' for removing "
+ "characters from the end")
+ << FixItHint::CreateReplacement(Assignment->getSourceRange(),
+ Replacement);
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
new file mode 100644
index 00000000000000..1a2054da1ed9cc
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
@@ -0,0 +1,39 @@
+//===--- StringViewSubstrCheck.h - clang-tidy---------------------*- C++
+//-*-===//
+//
+// 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_READABILITY_STRINGVIEWSUBSTRCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGVIEWSUBSTRCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Finds string_view substr() calls that can be replaced with remove_prefix()
+/// or remove_suffix().
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/readability/string-view-substr.html
+///
+/// The check matches two patterns:
+/// sv = sv.substr(N) -> sv.remove_prefix(N)
+/// sv = sv.substr(0, sv.length() - N) -> sv.remove_suffix(N)
+///
+/// These replacements make the intent clearer and are more efficient as they
+/// modify the string_view in place rather than creating a new one.
+class StringViewSubstrCheck : public ClangTidyCheck {
+public:
+ StringViewSubstrCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGVIEWSUBSTRCHECK_H
\ No newline at end of file
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 453a91e3b504cd..4eb6c98bcf461b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,13 @@ New checks
Gives warnings for tagged unions, where the number of tags is
different from the number of data members inside the union.
+- New :doc:`readability-string-view-substr
+ <clang-tidy/checks/readability/string-view-substr>` check.
+
+ Finds ``std::string_view::substr()`` calls that can be replaced with clearer
+ alternatives using ``remove_prefix()`` or ``remove_suffix()``. This makes the
+ intent clearer and is more efficient as it modifies the string_view in place.
+
- New :doc:`portability-template-virtual-member-function
<clang-tidy/checks/portability/template-virtual-member-function>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
new file mode 100644
index 00000000000000..f29bcbacc0d7d1
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - readability-string-view-substr
+
+readability-string-view-substr
+==============================
+
+Finds ``string_view substr()`` calls that can be replaced with clearer alternatives
+using ``remove_prefix()`` or ``remove_suffix()``.
+
+The check suggests the following transformations:
+
+=========================================== =======================
+Expression Replacement
+=========================================== =======================
+``sv = sv.substr(n)`` ``sv.remove_prefix(n)``
+``sv = sv.substr(0, sv.length()-n)`` ``sv.remove_suffix(n)``
+=========================================== =======================
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
new file mode 100644
index 00000000000000..274c305b640803
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s readability-stringview-substr %t
+
+namespace std {
+template <typename T>
+class basic_string_view {
+public:
+ using size_type = unsigned long;
+ static constexpr size_type npos = -1;
+
+ basic_string_view(const char*) {}
+ basic_string_view substr(size_type pos, size_type count = npos) const { return *this; }
+ void remove_prefix(size_type n) {}
+ void remove_suffix(size_type n) {}
+ size_type length() const { return 0; }
+
+ // Needed for assignment operator
+ basic_string_view& operator=(const basic_string_view&) { return *this; }
+};
+
+using string_view = basic_string_view<char>;
+}
+
+void test() {
+ std::string_view sv("test");
+ std::string_view sv1("test");
+ std::string_view sv2("other");
+
+ // Should match: Removing N characters from start
+ sv = sv.substr(5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_prefix' over 'substr' for removing characters from the start [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_prefix(5)
+
+ // Should match: Removing N characters from end
+ sv = sv.substr(0, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ // Should not match: Basic substring operations
+ sv = sv.substr(0, 3); // No warning - taking first N characters
+ sv = sv.substr(1, 3); // No warning - taking N characters from position 1
+
+ // Should not match: Operations on different string_views
+ sv = sv2.substr(0, sv2.length() - 3); // No warning - not self-assignment
+ sv = sv.substr(0, sv2.length() - 3); // No warning - length() from different string_view
+ sv1 = sv1.substr(0, sv2.length() - 3); // No warning - length() from different string_view
+ sv = sv1.substr(0, sv1.length() - 3); // No warning - not self-assignment
+
+ // Should not match: Not actually removing from end
+ sv = sv.substr(0, sv.length()); // No warning - keeping entire string
+ sv = sv.substr(0, sv.length() - 0); // No warning - subtracting zero
+
+ // Should not match: Complex expressions
+ sv = sv.substr(0, sv.length() - (3 + 2)); // No warning - complex arithmetic
+ sv = sv.substr(1 + 2, sv.length() - 3); // No warning - complex start position
+}
>From 64931c696fe64cdb8a7571029eea0cc40592a663 Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Mon, 16 Dec 2024 11:14:12 +0100
Subject: [PATCH 2/9] up
---
.../readability/StringViewSubstrCheck.cpp | 19 ++++++++-
.../readability/stringview_substr.cpp | 41 +++++++++++++++++++
2 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index e86a971695a835..01c3895f61f5c4 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -79,7 +79,24 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
}
// Case 2: Check for remove_suffix pattern
- if (StartText == "0") {
+ const Expr *Start = StartArg->IgnoreParenImpCasts();
+ bool IsZero = false;
+
+ if (const auto *IL = dyn_cast<IntegerLiteral>(Start)) {
+ IsZero = IL->getValue() == 0;
+ } else if (const auto *DRE = dyn_cast<DeclRefExpr>(Start)) {
+ // Check constants
+ if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+ if (VD->hasInit()) {
+ if (const auto *Init = dyn_cast<IntegerLiteral>(
+ VD->getInit()->IgnoreParenImpCasts())) {
+ IsZero = Init->getValue() == 0;
+ }
+ }
+ }
+ }
+
+ if (IsZero) {
if (const auto *BinOp = dyn_cast<BinaryOperator>(LengthArg)) {
if (BinOp->getOpcode() == BO_Sub) {
const Expr *LHS = BinOp->getLHS();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index 274c305b640803..2c1d9df3711b1d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -53,3 +53,44 @@ void test() {
sv = sv.substr(0, sv.length() - (3 + 2)); // No warning - complex arithmetic
sv = sv.substr(1 + 2, sv.length() - 3); // No warning - complex start position
}
+
+void test_zeros() {
+ std::string_view sv("test");
+ const int kZero = 0;
+ constexpr std::string_view::size_type Zero = 0; // Fixed: using string_view::size_type
+ #define START_POS 0
+
+ // All of these should match remove_suffix pattern and trigger warnings
+ sv = sv.substr(0, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr(kZero, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr(Zero, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr(START_POS, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr((0), sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr(0u, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ sv = sv.substr(0UL, sv.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ // These should NOT match the remove_suffix pattern
+ sv = sv.substr(1-1, sv.length() - 3); // No warning - complex expression
+ sv = sv.substr(sv.length() - 3, sv.length() - 3); // No warning - non-zero start
+}
+
>From 5b81a4d383f2e9751acedf9a895671828c2b683d Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Mon, 16 Dec 2024 20:49:30 +0100
Subject: [PATCH 3/9] up
---
.../readability/StringViewSubstrCheck.cpp | 148 ++++++++++++------
.../checks/readability/string-view-substr.rst | 2 +
.../readability/stringview_substr.cpp | 82 +++++-----
3 files changed, 149 insertions(+), 83 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index 01c3895f61f5c4..5c079e3203c989 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -45,13 +45,11 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
- // Get the DeclRefExpr for the target and source to compare variables
+ // Get the DeclRefExpr for the target and source
const auto *TargetDRE = dyn_cast<DeclRefExpr>(Target->IgnoreParenImpCasts());
const auto *SourceDRE = dyn_cast<DeclRefExpr>(Source->IgnoreParenImpCasts());
- // Only handle self-assignment cases
- if (!TargetDRE || !SourceDRE ||
- TargetDRE->getDecl() != SourceDRE->getDecl()) {
+ if (!TargetDRE || !SourceDRE) {
return;
}
@@ -59,89 +57,143 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
const Expr *LengthArg =
SubstrCall->getNumArgs() > 1 ? SubstrCall->getArg(1) : nullptr;
- // Get source text of first argument
std::string StartText =
Lexer::getSourceText(
CharSourceRange::getTokenRange(StartArg->getSourceRange()),
*Result.SourceManager, Result.Context->getLangOpts())
.str();
- // Case 1: Check for remove_prefix pattern - only when the second arg is
- // missing (uses npos)
+ const bool IsSameVar = (TargetDRE->getDecl() == SourceDRE->getDecl());
+
+ // Case 1: Check for remove_prefix pattern
if (!LengthArg || isa<CXXDefaultArgExpr>(LengthArg)) {
- std::string Replacement = TargetDRE->getNameInfo().getAsString() +
- ".remove_prefix(" + StartText + ")";
- diag(Assignment->getBeginLoc(), "prefer 'remove_prefix' over 'substr' for "
- "removing characters from the start")
- << FixItHint::CreateReplacement(Assignment->getSourceRange(),
- Replacement);
+ if (IsSameVar) {
+ std::string Replacement = TargetDRE->getNameInfo().getAsString() +
+ ".remove_prefix(" + StartText + ")";
+ diag(Assignment->getBeginLoc(), "prefer 'remove_prefix' over 'substr' "
+ "for removing characters from the start")
+ << FixItHint::CreateReplacement(Assignment->getSourceRange(),
+ Replacement);
+ }
return;
}
- // Case 2: Check for remove_suffix pattern
- const Expr *Start = StartArg->IgnoreParenImpCasts();
+ // Check if StartArg resolves to 0
bool IsZero = false;
- if (const auto *IL = dyn_cast<IntegerLiteral>(Start)) {
+ // Handle cases where StartArg is an integer literal
+ if (const auto *IL =
+ dyn_cast<IntegerLiteral>(StartArg->IgnoreParenImpCasts())) {
IsZero = IL->getValue() == 0;
- } else if (const auto *DRE = dyn_cast<DeclRefExpr>(Start)) {
- // Check constants
- if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
- if (VD->hasInit()) {
- if (const auto *Init = dyn_cast<IntegerLiteral>(
- VD->getInit()->IgnoreParenImpCasts())) {
- IsZero = Init->getValue() == 0;
- }
- }
- }
}
+ // Optional: Handle cases where StartArg evaluates to zero
+ if (!IsZero) {
+ // Add logic for other constant evaluation (e.g., constexpr evaluation)
+ const auto &EvalResult = StartArg->EvaluateKnownConstInt(*Result.Context);
+ IsZero = !EvalResult.isNegative() && EvalResult == 0;
+ }
+
+ // If StartArg resolves to 0, handle the case
if (IsZero) {
+ bool isFullCopy = false;
+
+ // Check for length() or length() - expr pattern
if (const auto *BinOp = dyn_cast<BinaryOperator>(LengthArg)) {
if (BinOp->getOpcode() == BO_Sub) {
const Expr *LHS = BinOp->getLHS();
const Expr *RHS = BinOp->getRHS();
- // Check if LHS is a length() call on the same string_view
+ // Check for length() call
if (const auto *LengthCall = dyn_cast<CXXMemberCallExpr>(LHS)) {
if (const auto *LengthMethod =
dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
if (LengthMethod->getName() == "length") {
- // Verify the length() call is on the same string_view
const Expr *LengthObject =
LengthCall->getImplicitObjectArgument();
const auto *LengthDRE =
dyn_cast<DeclRefExpr>(LengthObject->IgnoreParenImpCasts());
- if (!LengthDRE || LengthDRE->getDecl() != TargetDRE->getDecl()) {
+ if (!LengthDRE || LengthDRE->getDecl() != SourceDRE->getDecl()) {
return;
}
- // Must be a simple non-zero integer literal
- const auto *IL =
- dyn_cast<IntegerLiteral>(RHS->IgnoreParenImpCasts());
- if (!IL || IL->getValue() == 0) {
- return;
+ // Check if RHS is 0 or evaluates to 0
+ bool IsZero = false;
+ if (const auto *IL =
+ dyn_cast<IntegerLiteral>(RHS->IgnoreParenImpCasts())) {
+ IsZero = IL->getValue() == 0;
}
- std::string RHSText =
- Lexer::getSourceText(
- CharSourceRange::getTokenRange(RHS->getSourceRange()),
- *Result.SourceManager, Result.Context->getLangOpts())
- .str();
-
- std::string Replacement = TargetDRE->getNameInfo().getAsString() +
- ".remove_suffix(" + RHSText + ")";
- diag(Assignment->getBeginLoc(),
- "prefer 'remove_suffix' over 'substr' for removing "
- "characters from the end")
- << FixItHint::CreateReplacement(Assignment->getSourceRange(),
- Replacement);
- return;
+ if (IsZero) {
+ isFullCopy = true;
+ } else if (IsSameVar) {
+ // remove_suffix case (only for self-assignment)
+ std::string RHSText =
+ Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RHS->getSourceRange()),
+ *Result.SourceManager, Result.Context->getLangOpts())
+ .str();
+
+ std::string Replacement =
+ TargetDRE->getNameInfo().getAsString() + ".remove_suffix(" +
+ RHSText + ")";
+ diag(Assignment->getBeginLoc(),
+ "prefer 'remove_suffix' over 'substr' for removing "
+ "characters from the end")
+ << FixItHint::CreateReplacement(
+ Assignment->getSourceRange(), Replacement);
+ return;
+ }
}
}
}
}
+ } else if (const auto *LengthCall =
+ dyn_cast<CXXMemberCallExpr>(LengthArg)) {
+ // Handle direct length() call
+ if (const auto *LengthMethod =
+ dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
+ if (LengthMethod->getName() == "length") {
+ const Expr *LengthObject = LengthCall->getImplicitObjectArgument();
+ const auto *LengthDRE =
+ dyn_cast<DeclRefExpr>(LengthObject->IgnoreParenImpCasts());
+
+ if (LengthDRE && LengthDRE->getDecl() == SourceDRE->getDecl()) {
+ isFullCopy = true;
+ }
+ }
+ }
+ }
+ if (isFullCopy) {
+ if (IsSameVar) {
+ // Remove redundant self-copy, including the semicolon
+ SourceLocation EndLoc = Assignment->getEndLoc();
+ while (EndLoc.isValid()) {
+ const char *endPtr = Result.SourceManager->getCharacterData(EndLoc);
+ if (*endPtr == ';')
+ break;
+ EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager,
+ Result.Context->getLangOpts());
+ }
+ if (EndLoc.isValid()) {
+ diag(Assignment->getBeginLoc(), "redundant self-copy")
+ << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+ Assignment->getBeginLoc(),
+ EndLoc.getLocWithOffset(
+ 1))); // +1 to include the semicolon.
+ }
+ } else {
+ // Simplify copy between different variables
+ std::string Replacement = TargetDRE->getNameInfo().getAsString() +
+ " = " +
+ SourceDRE->getNameInfo().getAsString();
+ diag(Assignment->getBeginLoc(), "prefer direct copy over substr")
+ << FixItHint::CreateReplacement(Assignment->getSourceRange(),
+ Replacement);
+ }
+
+ return;
}
}
}
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
index f29bcbacc0d7d1..167d4f30cca8c3 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
@@ -13,4 +13,6 @@ Expression Replacement
=========================================== =======================
``sv = sv.substr(n)`` ``sv.remove_prefix(n)``
``sv = sv.substr(0, sv.length()-n)`` ``sv.remove_suffix(n)``
+``sv = sv.substr(0, sv.length())`` _Redundant self-copy_
+``sv1 = sv.substr(0, sv.length())`` ``sv1 = sv``
=========================================== =======================
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index 2c1d9df3711b1d..908c664e329b2f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -12,59 +12,62 @@ class basic_string_view {
void remove_prefix(size_type n) {}
void remove_suffix(size_type n) {}
size_type length() const { return 0; }
-
- // Needed for assignment operator
basic_string_view& operator=(const basic_string_view&) { return *this; }
};
using string_view = basic_string_view<char>;
-}
+} // namespace std
-void test() {
+void test_basic() {
std::string_view sv("test");
std::string_view sv1("test");
- std::string_view sv2("other");
+ std::string_view sv2("test");
- // Should match: Removing N characters from start
+ // Should match: remove_prefix
sv = sv.substr(5);
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_prefix' over 'substr' for removing characters from the start [readability-stringview-substr]
// CHECK-FIXES: sv.remove_prefix(5)
- // Should match: Removing N characters from end
+ // Should match: remove_suffix
sv = sv.substr(0, sv.length() - 3);
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
// CHECK-FIXES: sv.remove_suffix(3)
- // Should not match: Basic substring operations
- sv = sv.substr(0, 3); // No warning - taking first N characters
- sv = sv.substr(1, 3); // No warning - taking N characters from position 1
-
- // Should not match: Operations on different string_views
- sv = sv2.substr(0, sv2.length() - 3); // No warning - not self-assignment
- sv = sv.substr(0, sv2.length() - 3); // No warning - length() from different string_view
- sv1 = sv1.substr(0, sv2.length() - 3); // No warning - length() from different string_view
- sv = sv1.substr(0, sv1.length() - 3); // No warning - not self-assignment
-
- // Should not match: Not actually removing from end
- sv = sv.substr(0, sv.length()); // No warning - keeping entire string
- sv = sv.substr(0, sv.length() - 0); // No warning - subtracting zero
-
- // Should not match: Complex expressions
- sv = sv.substr(0, sv.length() - (3 + 2)); // No warning - complex arithmetic
- sv = sv.substr(1 + 2, sv.length() - 3); // No warning - complex start position
+ // Should match: remove_suffix with complex expression
+ sv = sv.substr(0, sv.length() - (3 + 2));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix((3 + 2))
+}
+
+void test_copies() {
+ std::string_view sv("test");
+ std::string_view sv1("test");
+ std::string_view sv2("test");
+
+ // Should match: remove redundant self copies
+ sv = sv.substr(0, sv.length());
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr]
+
+ sv = sv.substr(0, sv.length() - 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr]
+
+ // Should match: simplify copies between different variables
+ sv1 = sv.substr(0, sv.length());
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer direct copy over substr [readability-stringview-substr]
+ // CHECK-FIXES: sv1 = sv
+
+ sv2 = sv.substr(0, sv.length() - 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer direct copy over substr [readability-stringview-substr]
+ // CHECK-FIXES: sv2 = sv
}
-void test_zeros() {
+void test_zero_forms() {
std::string_view sv("test");
const int kZero = 0;
- constexpr std::string_view::size_type Zero = 0; // Fixed: using string_view::size_type
+ constexpr std::string_view::size_type Zero = 0;
#define START_POS 0
-
- // All of these should match remove_suffix pattern and trigger warnings
- sv = sv.substr(0, sv.length() - 3);
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
- // CHECK-FIXES: sv.remove_suffix(3)
+ // Should match: various forms of zero in first argument
sv = sv.substr(kZero, sv.length() - 3);
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
// CHECK-FIXES: sv.remove_suffix(3)
@@ -88,9 +91,18 @@ void test_zeros() {
sv = sv.substr(0UL, sv.length() - 3);
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
// CHECK-FIXES: sv.remove_suffix(3)
-
- // These should NOT match the remove_suffix pattern
- sv = sv.substr(1-1, sv.length() - 3); // No warning - complex expression
- sv = sv.substr(sv.length() - 3, sv.length() - 3); // No warning - non-zero start
}
+void test_should_not_match() {
+ std::string_view sv("test");
+ std::string_view sv1("test");
+ std::string_view sv2("test");
+
+ // No match: substr used for prefix or mid-view
+ sv = sv.substr(1, sv.length() - 3); // No warning
+
+ // No match: Different string_views
+ sv = sv2.substr(0, sv2.length() - 3); // No warning
+
+
+}
>From aa01bc040241a18de12d261bba60836f66b55839 Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Mon, 16 Dec 2024 21:43:25 +0100
Subject: [PATCH 4/9] up
---
.../docs/clang-tidy/checks/readability/string-view-substr.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
index 167d4f30cca8c3..c4e25a1d621c8f 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
@@ -13,6 +13,6 @@ Expression Replacement
=========================================== =======================
``sv = sv.substr(n)`` ``sv.remove_prefix(n)``
``sv = sv.substr(0, sv.length()-n)`` ``sv.remove_suffix(n)``
-``sv = sv.substr(0, sv.length())`` _Redundant self-copy_
+``sv = sv.substr(0, sv.length())`` Redundant self-copy
``sv1 = sv.substr(0, sv.length())`` ``sv1 = sv``
=========================================== =======================
>From f60ec8ed207798800e984d01db28e01c9a5215a8 Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Tue, 17 Dec 2024 09:27:01 +0100
Subject: [PATCH 5/9] Update StringViewSubstrCheck.h
---
.../clang-tidy/readability/StringViewSubstrCheck.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
index 1a2054da1ed9cc..365594d815f56a 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
@@ -36,4 +36,4 @@ class StringViewSubstrCheck : public ClangTidyCheck {
} // namespace clang::tidy::readability
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGVIEWSUBSTRCHECK_H
\ No newline at end of file
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGVIEWSUBSTRCHECK_H
>From e48db3491fa350df17cfece12752256247339bbb Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Wed, 1 Jan 2025 22:11:54 +0100
Subject: [PATCH 6/9] feedback
---
.../readability/StringViewSubstrCheck.cpp | 6 ++---
.../readability/StringViewSubstrCheck.h | 5 ++++
.../readability/stringview_substr.cpp | 25 ++++++++++++++++++-
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index 5c079e3203c989..bac07fa5b5ab7a 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -108,7 +108,7 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *LengthCall = dyn_cast<CXXMemberCallExpr>(LHS)) {
if (const auto *LengthMethod =
dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
- if (LengthMethod->getName() == "length") {
+ if (LengthMethod->getName() == "length" || LengthMethod->getName() == "size") {
const Expr *LengthObject =
LengthCall->getImplicitObjectArgument();
const auto *LengthDRE =
@@ -151,10 +151,10 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
}
} else if (const auto *LengthCall =
dyn_cast<CXXMemberCallExpr>(LengthArg)) {
- // Handle direct length() call
+ // Handle direct length() or size() call
if (const auto *LengthMethod =
dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
- if (LengthMethod->getName() == "length") {
+ if (LengthMethod->getName() == "length" || LengthMethod->getName() == "size") {
const Expr *LengthObject = LengthCall->getImplicitObjectArgument();
const auto *LengthDRE =
dyn_cast<DeclRefExpr>(LengthObject->IgnoreParenImpCasts());
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
index 365594d815f56a..d4f6ae3ff20695 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
@@ -23,6 +23,8 @@ namespace clang::tidy::readability {
/// The check matches two patterns:
/// sv = sv.substr(N) -> sv.remove_prefix(N)
/// sv = sv.substr(0, sv.length() - N) -> sv.remove_suffix(N)
+/// sv = sv.substr(0, sv.length()) -> // Remove redundant self-copy
+/// sv1 = sv2.substr(0, sv2.length()) -> sv1 = sv2
///
/// These replacements make the intent clearer and are more efficient as they
/// modify the string_view in place rather than creating a new one.
@@ -32,6 +34,9 @@ class StringViewSubstrCheck : public ClangTidyCheck {
: 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::readability
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index 908c664e329b2f..a6c0897f393c50 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-stringview-substr %t
+// RUN: %check_clang_tidy %s readability-stringview-substr -std=c++17 %t
namespace std {
template <typename T>
@@ -12,6 +12,7 @@ class basic_string_view {
void remove_prefix(size_type n) {}
void remove_suffix(size_type n) {}
size_type length() const { return 0; }
+ size_type size() const { return 0; }
basic_string_view& operator=(const basic_string_view&) { return *this; }
};
@@ -39,6 +40,28 @@ void test_basic() {
// CHECK-FIXES: sv.remove_suffix((3 + 2))
}
+void test_size_method() {
+ std::string_view sv("test");
+ std::string_view sv2("test");
+
+ // Should match: remove_suffix with size()
+ sv = sv.substr(0, sv.size() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr]
+ // CHECK-FIXES: sv.remove_suffix(3)
+
+ // Should match: remove redundant self copies
+ sv = sv.substr(0, sv.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr]
+
+ sv = sv.substr(0, sv.size() - 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr]
+
+ // Should match: simplify copies between different variables
+ sv2 = sv.substr(0, sv.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer direct copy over substr [readability-stringview-substr]
+ // CHECK-FIXES: sv2 = sv
+}
+
void test_copies() {
std::string_view sv("test");
std::string_view sv1("test");
>From 5ffdd872240f3b2840012d0b388ef4d2d904d148 Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Wed, 1 Jan 2025 22:16:47 +0100
Subject: [PATCH 7/9] feedback
---
.../readability/StringViewSubstrCheck.cpp | 8 +++++---
.../checkers/readability/stringview_substr.cpp | 17 +++++++++++++++++
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index bac07fa5b5ab7a..944b0bd9da9e98 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -22,7 +22,7 @@ void StringViewSubstrCheck::registerMatchers(MatchFinder *Finder) {
// Match assignment to string_view's substr
Finder->addMatcher(
cxxOperatorCallExpr(
- hasOverloadedOperatorName("="),
+ unless(isInTemplateInstantiation()), hasOverloadedOperatorName("="),
hasArgument(0, expr(HasStringViewType).bind("target")),
hasArgument(
1, cxxMemberCallExpr(callee(memberExpr(hasDeclaration(
@@ -108,7 +108,8 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *LengthCall = dyn_cast<CXXMemberCallExpr>(LHS)) {
if (const auto *LengthMethod =
dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
- if (LengthMethod->getName() == "length" || LengthMethod->getName() == "size") {
+ if (LengthMethod->getName() == "length" ||
+ LengthMethod->getName() == "size") {
const Expr *LengthObject =
LengthCall->getImplicitObjectArgument();
const auto *LengthDRE =
@@ -154,7 +155,8 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
// Handle direct length() or size() call
if (const auto *LengthMethod =
dyn_cast<CXXMethodDecl>(LengthCall->getDirectCallee())) {
- if (LengthMethod->getName() == "length" || LengthMethod->getName() == "size") {
+ if (LengthMethod->getName() == "length" ||
+ LengthMethod->getName() == "size") {
const Expr *LengthObject = LengthCall->getImplicitObjectArgument();
const auto *LengthDRE =
dyn_cast<DeclRefExpr>(LengthObject->IgnoreParenImpCasts());
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index a6c0897f393c50..50a59b7273c525 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -62,6 +62,23 @@ void test_size_method() {
// CHECK-FIXES: sv2 = sv
}
+template <typename T>
+void test_template_instantiation() {
+ std::basic_string_view<T> sv("test");
+ std::basic_string_view<T> sv2("test");
+
+ // Should not match: inside a template instantiation
+ sv = sv.substr(0, sv.size() - 3); // No warning
+ sv = sv.substr(0, sv.length()); // No warning
+ sv2 = sv.substr(0, sv.size()); // No warning
+}
+
+// No matches when instantiated
+void test_template_no_matches() {
+ test_template_instantiation<char>(); // No warnings
+ test_template_instantiation<wchar_t>(); // No warnings
+}
+
void test_copies() {
std::string_view sv("test");
std::string_view sv1("test");
>From 0617301ef3408fd19f7a9a34e67e032728f418c5 Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Fri, 3 Jan 2025 22:49:22 +0100
Subject: [PATCH 8/9] woop woop
---
.../readability/StringViewSubstrCheck.cpp | 34 ++++++++++++-------
.../readability/stringview_substr.cpp | 9 ++++-
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index 944b0bd9da9e98..36c4b7abbc1947 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -16,20 +16,28 @@ using namespace clang::ast_matchers;
namespace clang::tidy::readability {
void StringViewSubstrCheck::registerMatchers(MatchFinder *Finder) {
- const auto HasStringViewType = hasType(hasUnqualifiedDesugaredType(recordType(
- hasDeclaration(recordDecl(hasName("::std::basic_string_view"))))));
+ // Match string_view type
+ const auto StringViewDecl = recordDecl(hasName("::std::basic_string_view"));
+ const auto IsStringView = qualType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(StringViewDecl))));
- // Match assignment to string_view's substr
+ // Match substr() call on string_views
+ const auto SubstrCall = cxxMemberCallExpr(
+ callee(memberExpr(hasDeclaration(cxxMethodDecl(hasName("substr"))))),
+ on(expr(hasType(IsStringView)).bind("source")));
+
+ // Match assignment to string_view's substr, but exclude assignments in expressions
Finder->addMatcher(
- cxxOperatorCallExpr(
- unless(isInTemplateInstantiation()), hasOverloadedOperatorName("="),
- hasArgument(0, expr(HasStringViewType).bind("target")),
- hasArgument(
- 1, cxxMemberCallExpr(callee(memberExpr(hasDeclaration(
- cxxMethodDecl(hasName("substr"))))),
- on(expr(HasStringViewType).bind("source")))
- .bind("substr_call")))
- .bind("assignment"),
+ stmt(
+ cxxOperatorCallExpr(
+ unless(isInTemplateInstantiation()),
+ hasOverloadedOperatorName("="),
+ hasArgument(0, expr(hasType(IsStringView)).bind("target")),
+ hasArgument(1, SubstrCall.bind("substr_call")))
+ .bind("assignment"),
+ // Exclude assignments used in larger expressions
+ unless(hasAncestor(varDecl())),
+ unless(hasAncestor(callExpr()))),
this);
}
@@ -45,6 +53,8 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
+
+
// Get the DeclRefExpr for the target and source
const auto *TargetDRE = dyn_cast<DeclRefExpr>(Target->IgnoreParenImpCasts());
const auto *SourceDRE = dyn_cast<DeclRefExpr>(Source->IgnoreParenImpCasts());
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index 50a59b7273c525..9e389b5058b555 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -143,6 +143,13 @@ void test_should_not_match() {
// No match: Different string_views
sv = sv2.substr(0, sv2.length() - 3); // No warning
-
}
+
+void f(std::string_view) {}
+void test_expr_with_cleanups() {
+ std::string_view sv("test");
+ const auto copy = sv = sv.substr(0, sv.length() - 3); // No warning
+ f(sv = sv.substr(0, sv.length() - 3)); // No warning
+}
+
>From afabddb85c37916a67715687ca308096dd49a4bd Mon Sep 17 00:00:00 2001
From: Helmut Januschka <helmut at januschka.com>
Date: Tue, 7 Jan 2025 23:54:31 +0100
Subject: [PATCH 9/9] feedback
---
.../readability/StringViewSubstrCheck.cpp | 48 ++++++++++---------
.../readability/StringViewSubstrCheck.h | 1 +
.../checks/readability/string-view-substr.rst | 7 +--
.../readability/stringview_substr.cpp | 8 ++--
4 files changed, 36 insertions(+), 28 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
index 36c4b7abbc1947..de931e8e2f8149 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -18,26 +18,25 @@ namespace clang::tidy::readability {
void StringViewSubstrCheck::registerMatchers(MatchFinder *Finder) {
// Match string_view type
const auto StringViewDecl = recordDecl(hasName("::std::basic_string_view"));
- const auto IsStringView = qualType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(StringViewDecl))));
+ const auto IsStringView = qualType(
+ hasUnqualifiedDesugaredType(recordType(hasDeclaration(StringViewDecl))));
// Match substr() call on string_views
const auto SubstrCall = cxxMemberCallExpr(
callee(memberExpr(hasDeclaration(cxxMethodDecl(hasName("substr"))))),
on(expr(hasType(IsStringView)).bind("source")));
- // Match assignment to string_view's substr, but exclude assignments in expressions
+ // Match assignment to string_view's substr, but exclude assignments in
+ // expressions
Finder->addMatcher(
- stmt(
- cxxOperatorCallExpr(
- unless(isInTemplateInstantiation()),
- hasOverloadedOperatorName("="),
- hasArgument(0, expr(hasType(IsStringView)).bind("target")),
- hasArgument(1, SubstrCall.bind("substr_call")))
- .bind("assignment"),
- // Exclude assignments used in larger expressions
- unless(hasAncestor(varDecl())),
- unless(hasAncestor(callExpr()))),
+ stmt(cxxOperatorCallExpr(
+ unless(isInTemplateInstantiation()),
+ hasOverloadedOperatorName("="),
+ hasArgument(0, expr(hasType(IsStringView)).bind("target")),
+ hasArgument(1, SubstrCall.bind("substr_call")))
+ .bind("assignment"),
+ // Exclude assignments used in larger expressions
+ unless(hasAncestor(varDecl())), unless(hasAncestor(callExpr()))),
this);
}
@@ -53,8 +52,6 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
return;
}
-
-
// Get the DeclRefExpr for the target and source
const auto *TargetDRE = dyn_cast<DeclRefExpr>(Target->IgnoreParenImpCasts());
const auto *SourceDRE = dyn_cast<DeclRefExpr>(Source->IgnoreParenImpCasts());
@@ -138,20 +135,28 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
if (IsZero) {
isFullCopy = true;
- } else if (IsSameVar) {
- // remove_suffix case (only for self-assignment)
+ } else {
+ // remove_suffix case (works for both self and different vars)
std::string RHSText =
Lexer::getSourceText(
CharSourceRange::getTokenRange(RHS->getSourceRange()),
*Result.SourceManager, Result.Context->getLangOpts())
.str();
+ std::string TargetName = TargetDRE->getNameInfo().getAsString();
+ std::string SourceName = SourceDRE->getNameInfo().getAsString();
+
std::string Replacement =
- TargetDRE->getNameInfo().getAsString() + ".remove_suffix(" +
- RHSText + ")";
+ IsSameVar
+ ? (TargetName + ".remove_suffix(" + RHSText + ")")
+ : (TargetName + " = " + SourceName + ";\n" +
+ TargetName + ".remove_suffix(" + RHSText + ")");
+
diag(Assignment->getBeginLoc(),
- "prefer 'remove_suffix' over 'substr' for removing "
- "characters from the end")
+ IsSameVar
+ ? "prefer 'remove_suffix' over 'substr' for removing "
+ "characters from the end"
+ : "prefer assignment and remove_suffix over substr")
<< FixItHint::CreateReplacement(
Assignment->getSourceRange(), Replacement);
return;
@@ -204,7 +209,6 @@ void StringViewSubstrCheck::check(const MatchFinder::MatchResult &Result) {
<< FixItHint::CreateReplacement(Assignment->getSourceRange(),
Replacement);
}
-
return;
}
}
diff --git a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
index d4f6ae3ff20695..ffdee9afa5395e 100644
--- a/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
@@ -25,6 +25,7 @@ namespace clang::tidy::readability {
/// sv = sv.substr(0, sv.length() - N) -> sv.remove_suffix(N)
/// sv = sv.substr(0, sv.length()) -> // Remove redundant self-copy
/// sv1 = sv2.substr(0, sv2.length()) -> sv1 = sv2
+/// sv1 = sv2.substr(0, sv2.length() - N) -> sv1 = sv2; sv1.remove_suffix(N)
///
/// These replacements make the intent clearer and are more efficient as they
/// modify the string_view in place rather than creating a new one.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
index c4e25a1d621c8f..45b9bf0a787a76 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-view-substr.rst
@@ -8,11 +8,12 @@ using ``remove_prefix()`` or ``remove_suffix()``.
The check suggests the following transformations:
-=========================================== =======================
+=========================================== =======================================
Expression Replacement
-=========================================== =======================
+=========================================== =======================================
``sv = sv.substr(n)`` ``sv.remove_prefix(n)``
``sv = sv.substr(0, sv.length()-n)`` ``sv.remove_suffix(n)``
``sv = sv.substr(0, sv.length())`` Redundant self-copy
``sv1 = sv.substr(0, sv.length())`` ``sv1 = sv``
-=========================================== =======================
+``sv1 = sv2.substr(0, sv2.length()-n)`` ``sv1 = sv2;`` ``sv1.remove_suffix(n)``
+=========================================== =======================================
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
index 9e389b5058b555..ca39d9b72badef 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/stringview_substr.cpp
@@ -141,9 +141,11 @@ void test_should_not_match() {
// No match: substr used for prefix or mid-view
sv = sv.substr(1, sv.length() - 3); // No warning
- // No match: Different string_views
- sv = sv2.substr(0, sv2.length() - 3); // No warning
-
+ // Should match: Different string_views with remove_suffix pattern
+ sv = sv2.substr(0, sv2.length() - 3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer assignment and remove_suffix over substr [readability-stringview-substr]
+ // CHECK-FIXES: sv = sv2;
+ // CHECK-FIXES: sv.remove_suffix(3)
}
void f(std::string_view) {}
More information about the cfe-commits
mailing list