[clang-tools-extra] [clang-tidy] Add readability-string-view-substr check (PR #120055)
Helmut Januschka via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 16 00:50:51 PST 2024
https://github.com/hjanuschka created https://github.com/llvm/llvm-project/pull/120055
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.
>From 16a98e0d99ccee0c2bb6bad51c27c67c5c43d747 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] [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 | 105 ++++++++++++++++++
.../readability/StringViewSubstrCheck.h | 38 +++++++
clang-tools-extra/docs/ReleaseNotes.rst | 7 ++
.../checks/readability/string-view-substr.rst | 16 +++
.../readability/stringview_substr.cpp | 55 +++++++++
7 files changed, 225 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..0889fbbd86a281 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..5ed96480c0c6f2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.cpp
@@ -0,0 +1,105 @@
+#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..0a3202d3d934cf
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/StringViewSubstrCheck.h
@@ -0,0 +1,38 @@
+//===--- 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..2e8eec39559c95
--- /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
+}
More information about the cfe-commits
mailing list