[clang-tidy] Add readability-use-span-first-last check #118074

hjanuschka · 2024-11-29T09:21:06Z

Add new clang-tidy check that suggests using std::span's more expressive first() and last() member functions instead of equivalent subspan() calls.

These dedicated methods were added to the standard to provide clearer alternatives to common subspan operations. They improve readability by better expressing intent and are less error-prone by eliminating manual offset calculations.

For example:

std::span<int> s = ...;
auto sub1 = s.subspan(0, n);           // transforms to: auto sub1 = s.first(n);
auto sub2 = s.subspan(s.size() - n);   // transforms to: auto sub2 = s.last(n);
auto sub3 = s.subspan(1, n);           // not transformed, no clearer alternative
auto sub4 = s.subspan(n);              // not transformed, no clearer alternative

not sure if this is a readability or a modernize check ❓

Add new check that modernizes std::span::subspan() calls to use the more expressive first() and last() member functions where applicable.

llvmbot · 2024-11-29T09:21:39Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Helmut Januschka (hjanuschka)

Changes

Add new clang-tidy check that suggests using std::span's more expressive first() and last() member functions instead of equivalent subspan() calls.

The check modernizes code by replacing:

subspan(0, n) -> first(n)
subspan(n) -> last(size() - n)

These dedicated methods were added to the standard to provide clearer alternatives to common subspan operations. They improve readability by better expressing intent and are less error-prone by eliminating manual offset calculations.

For example:

std::span&lt;int&gt; s = ...;
auto sub1 = s.subspan(0, n);     // transforms to: auto sub1 = s.first(n);
auto sub2 = s.subspan(n);        // transforms to: auto sub2 = s.last(s.size() - n);
auto sub3 = s.subspan(1, n);     // not transformed, no direct equivalent

not sure if this is a readability or a modernize check ❓

Full diff: https://github.com/llvm/llvm-project/pull/118074.diff

7 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
(modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3)
(added) clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.cpp (+98)
(added) clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h (+40)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
(added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-span-first-last.rst (+19)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/modernize-subspan-conversion.cpp (+50)

diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index c919d49b42873a..47dd12a2640b6c 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -49,6 +49,7 @@ add_clang_library(clangTidyModernizeModule STATIC
   UseTransparentFunctorsCheck.cpp
   UseUncaughtExceptionsCheck.cpp
   UseUsingCheck.cpp
+  UseSpanFirstLastCheck.cpp
 
   LINK_LIBS
   clangTidy
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index 18607593320635..6fc5de5aad20b7 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -42,6 +42,7 @@
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseRangesCheck.h"
+#include "UseSpanFirstLastCheck.h"
 #include "UseStartsEndsWithCheck.h"
 #include "UseStdFormatCheck.h"
 #include "UseStdNumbersCheck.h"
@@ -122,6 +123,8 @@ class ModernizeModule : public ClangTidyModule {
     CheckFactories.registerCheck<UseUncaughtExceptionsCheck>(
         "modernize-use-uncaught-exceptions");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
+    CheckFactories.registerCheck<UseSpanFirstLastCheck>("modernize-use-span-first-last");
+
   }
 };
 
diff --git a/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.cpp
new file mode 100644
index 00000000000000..6cf01386b0c3fb
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.cpp
@@ -0,0 +1,98 @@
+//===--- UseSpanFirstLastCheck.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 "UseSpanFirstLastCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::modernize {
+
+void UseSpanFirstLastCheck::registerMatchers(MatchFinder *Finder) {
+  // Match span::subspan calls
+  const auto HasSpanType =
+      hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+          classTemplateSpecializationDecl(hasName("::std::span"))))));
+
+  Finder->addMatcher(cxxMemberCallExpr(callee(memberExpr(hasDeclaration(
+                                           cxxMethodDecl(hasName("subspan"))))),
+                                       on(expr(HasSpanType)))
+                         .bind("subspan"),
+                     this);
+}
+
+void UseSpanFirstLastCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("subspan");
+  if (!Call)
+    return;
+
+  handleSubspanCall(Result, Call);
+}
+
+void UseSpanFirstLastCheck::handleSubspanCall(
+    const MatchFinder::MatchResult &Result, const CXXMemberCallExpr *Call) {
+  // Get arguments
+  unsigned NumArgs = Call->getNumArgs();
+  if (NumArgs == 0 || NumArgs > 2)
+    return;
+
+  const Expr *Offset = Call->getArg(0);
+  const Expr *Count = NumArgs > 1 ? Call->getArg(1) : nullptr;
+  auto &Context = *Result.Context;
+  bool IsZeroOffset = false;
+
+  // Check if offset is zero through any implicit casts
+  const Expr *OffsetE = Offset->IgnoreImpCasts();
+  if (const auto *IL = dyn_cast<IntegerLiteral>(OffsetE)) {
+    IsZeroOffset = IL->getValue() == 0;
+  }
+
+  // Build replacement text
+  std::string Replacement;
+  if (IsZeroOffset && Count) {
+    // subspan(0, count) -> first(count)
+    auto CountStr = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Count->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+    const auto *Base =
+        cast<CXXMemberCallExpr>(Call)->getImplicitObjectArgument();
+    auto BaseStr = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Base->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+    Replacement = BaseStr.str() + ".first(" + CountStr.str() + ")";
+  } else if (NumArgs == 1) {
+    // subspan(n) -> last(size() - n)
+    auto OffsetStr = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Offset->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+
+    const auto *Base =
+        cast<CXXMemberCallExpr>(Call)->getImplicitObjectArgument();
+    auto BaseStr = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Base->getSourceRange()),
+        Context.getSourceManager(), Context.getLangOpts());
+
+    Replacement = BaseStr.str() + ".last(" + BaseStr.str() + ".size() - " +
+                  OffsetStr.str() + ")";
+  }
+
+  if (!Replacement.empty()) {
+    if (IsZeroOffset && Count) {
+      diag(Call->getBeginLoc(), "prefer span::first() over subspan()")
+          << FixItHint::CreateReplacement(Call->getSourceRange(), Replacement);
+    } else {
+      diag(Call->getBeginLoc(), "prefer span::last() over subspan()")
+          << FixItHint::CreateReplacement(Call->getSourceRange(), Replacement);
+    }
+  }
+}
+
+} // namespace clang::tidy::modernize
\ No newline at end of file
diff --git a/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h b/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h
new file mode 100644
index 00000000000000..8d4c6035f7ec76
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h
@@ -0,0 +1,40 @@
+//===--- UseSpanFirstLastCheck.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_MODERNIZE_USESPANFIRSTLASTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESPANFIRSTLASTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::modernize {
+
+/// Converts std::span::subspan() calls to the more modern first()/last()
+/// methods where applicable.
+///
+/// For example:
+/// \code
+///   std::span<int> s = ...;
+///   auto sub = s.subspan(0, n);    // ->  auto sub = s.first(n);
+///   auto sub2 = s.subspan(n);      // ->  auto sub2 = s.last(s.size() - n);
+/// \endcode
+class UseSpanFirstLastCheck : public ClangTidyCheck {
+public:
+  UseSpanFirstLastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void handleSubspanCall(const ast_matchers::MatchFinder::MatchResult &Result,
+                         const CXXMemberCallExpr *Call);
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESPANFIRSTLASTCHECK_H
\ No newline at end of file
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f050391110385e..04a45d002c0d1d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -145,6 +145,10 @@ New checks
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
+- New check `modernize-use-span-first-last` has been added that suggests using
+  ``std::span::first()`` and ``std::span::last()`` member functions instead of
+  equivalent ``subspan()``.
+
 - New alias :doc:`cert-arr39-c <clang-tidy/checks/cert/arr39-c>` to
   :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone/sizeof-expression>` was added.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-span-first-last.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-span-first-last.rst
new file mode 100644
index 00000000000000..e8aad59bb2264f
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-span-first-last.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - modernize-use-span-first-last
+
+modernize-use-span-first-last
+============================
+
+Checks for uses of ``std::span::subspan()`` that can be replaced with clearer
+``first()`` or ``last()`` member functions.
+
+Covered scenarios:
+
+==================================== ==================================
+Expression                           Replacement
+------------------------------------ ----------------------------------
+``s.subspan(0, n)``                  ``s.first(n)``
+``s.subspan(n)``                     ``s.last(s.size() - n)``
+==================================== ==================================
+
+Non-zero offset with count (like ``subspan(1, n)``) has no direct equivalent
+using ``first()`` or ``last()``, so these cases are not transformed.
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/modernize-subspan-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/modernize-subspan-conversion.cpp
new file mode 100644
index 00000000000000..cb78bc02f22d4f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/modernize-subspan-conversion.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s modernize-use-span-first-last %t
+
+namespace std {
+template <typename T>
+class span {
+  T* ptr;
+  __SIZE_TYPE__ len;
+
+public:
+  span(T* p, __SIZE_TYPE__ l) : ptr(p), len(l) {}
+  
+  span<T> subspan(__SIZE_TYPE__ offset) const {
+    return span(ptr + offset, len - offset);
+  }
+  
+  span<T> subspan(__SIZE_TYPE__ offset, __SIZE_TYPE__ count) const {
+    return span(ptr + offset, count);
+  }
+
+  span<T> first(__SIZE_TYPE__ count) const {
+    return span(ptr, count);
+  }
+
+  span<T> last(__SIZE_TYPE__ count) const {
+    return span(ptr + (len - count), count);
+  }
+
+  __SIZE_TYPE__ size() const { return len; }
+};
+} // namespace std
+
+void test() {
+  int arr[] = {1, 2, 3, 4, 5};
+  std::span<int> s(arr, 5);
+
+  auto sub1 = s.subspan(0, 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer span::first() over subspan()
+  // CHECK-FIXES: auto sub1 = s.first(3);
+
+  auto sub2 = s.subspan(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer span::last() over subspan()
+  // CHECK-FIXES: auto sub2 = s.last(s.size() - 2);
+
+  __SIZE_TYPE__ n = 2;
+  auto sub3 = s.subspan(0, n);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer span::first() over subspan()
+  // CHECK-FIXES: auto sub3 = s.first(n);
+
+  auto sub4 = s.subspan(1, 2);  // No warning
+}
\ No newline at end of file

github-actions · 2024-11-29T09:24:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

carlosgalvezp · 2024-11-29T09:26:28Z

not sure if this is a readability or a modernize check

readability. modernize is for using new features from newer standards.

carlosgalvezp · 2024-11-29T09:31:10Z

auto sub2 = s.subspan(n);        // transforms to: auto sub2 = s.last(s.size() - n);

IMHO the transformed code is less readable. It would be more useful to do the opposite transformation, maybe that's what you intended?

auto sub2 = s.subspan(s.size() - n);        // transforms to: auto sub2 = s.last(n);

clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h

hjanuschka · 2024-11-29T10:14:33Z

@carlosgalvezp thanks for the feedback. I've removed the transformation:

auto sub2 = s.subspan(n);        // transforms to: auto sub2 = s.last(s.size() - n);

While I agree that the transformed expression is more complex (due to size() - x), I initially included it for two reasons:

Consistency: Using first()/last() everywhere possible makes the codebase more uniform
The method name last() better expresses the intent than subspan(), even if the calculation is more verbose

Let me know if you think we should revisit this decision.

EugeneZelenko · 2024-11-29T15:54:17Z

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

  UppercaseLiteralSuffixCheck.cpp
  UseAnyOfAllOfCheck.cpp
  UseStdMinMaxCheck.cpp
+  UseSpanFirstLastCheck.cpp


Please keep alphabetical order.

fixed, with the 2 "Use" (https://github.com/llvm/llvm-project/pull/118074/files/c7fd4d5f508d43cd7fdaf7ed9a0006687a4215ed#diff-4be36b5009d110fd7bf319b8b06cf0e86d91b04f4c30dcd8aaf94825626b2e2aL26) filenames, but there is RedundantInlineSpecifierCheck also wrong, should i fix this in the same PR?

I think one-line change is fine in this pull request.

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst

clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp

pkasting · 2024-11-29T17:06:13Z

FWIW I agree that last(size() - n) is worse than subspan(n), and that the reverse transform (subspan(size() - n) -> last(n)) is better.

5chmidti

It would be great to have this check work with uninstantiated templates and use TK_TraversUnlessSpelledInSource as well.

Here is an example of a matcher for the uninstantiated case:
https://godbolt.org/z/1xnGGj7z9

I couldn't fully write it, because there is no matcher to get the member information and constrain it to subspan.

Please also adjust your PR comment to reflect the change for what pattern is matched in the last case.

Overall a good readability improvement check, nice

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp

….cpp Co-authored-by: EugeneZelenko <[email protected]>

Co-authored-by: EugeneZelenko <[email protected]>

clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst

hjanuschka · 2024-12-04T20:40:12Z

@EugeneZelenko CI fails because of

ReleaseNotes.rst:145:unknown document: 'clang-tidy/checks/readability/readability-use-span-first-last'

could you help me, i don't get it why this fails, bet its a small thing but i am too blind for this.

also, since i have troubles doing the RST's is there any documentation on how to run the "docs-CI" locally (to prevent me from pushing wrong commits back and forth 🙈 in the hope to get a green light)

carlosgalvezp · 2024-12-04T20:50:31Z

You have duplicated readability when referring to the document:

- 'clang-tidy/checks/readability/readability-use-span-first-last'
+ 'clang-tidy/checks/readability/use-span-first-last'

You can build them with: ninja docs-clang-tools-html, with following CMake config:

cmake -B build -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_ENABLE_SPHINX=ON ./llvm

You can see that's the actual command the job is running:
https://github.com/llvm/llvm-project/actions/runs/12167816720/job/33937230310?pr=118074

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

HerrCai0907

It may still work on TraversalKind::TK_IgnoreUnlessSpelledInSource. Could you try to set traversal mode in getCheckTraversalKind. It can avoid noise of something like implicit cast.

hjanuschka · 2024-12-05T08:07:20Z

feedback addressed.

@5chmidti - added range support
@HerrCai0907 tried using TK_IgnoreUnlessSpelledInSource but this breaks the template based cases.
i am afraid having TK_IgnoreUnlessSpelledInSource would require way more complex matchers? how big of a deal is to not use it?

HerrCai0907 · 2024-12-05T09:47:49Z

tried using TK_IgnoreUnlessSpelledInSource but this breaks the template based cases.

Thanks! If it will waste you more effort, it's good to keep it like this.

hjanuschka · 2024-12-05T11:42:11Z

wasting effort would not be the problem (it never is; learning is the goal) - right now i just dont understand the difference, and therefore dont see the benefit, as far as i see it would introduce more complicated code to still handle templates.

clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst

5chmidti

wasting effort would not be the problem (it never is; learning is the goal) - right now i just dont understand the difference, and therefore dont see the benefit, as far as i see it would introduce more complicated code to still handle templates.

TK_IgnoreUnlessSpelledInSource changes how the AST is seen by excluding AST nodes that are implicit, e.g., implicit template instantiations or implicit casts. This can in some cases improve the readability of the check by simplifying the matcher, or improve performance because not every implicit AST node, such as instantiated templates, are traversed and checked. The effect of this varies a lot, depending on what a check is trying to achieve. Checkout the Traverse Mode section on top of the AST matcher reference for more details, and importantly, a lot of examples on the differences.

For this check, the benefit will be performance, and not readability. It will be possible to skip template instantiations, of which there may be many because span is the generic view on contiguous memory (joined by mdspan). Overall, the checking done by this check is not too costly, so there would not be a massive difference.

Godbolt with the AST view and clang-query are great to iteratively build up a matcher, checkout https://godbolt.org/z/GrfMTxeGa for matching uninstantiated templates with TK_IgnoreUnlessSpelledInSource

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

hjanuschka

ready for re-review

5chmidti

You could try to implement the check with TK_TraverseUnlessSpelledInSource if you'd like, which would only add two more matchers via addMatcher, and the traversal kind function (https://godbolt.org/z/GrfMTxeGa). IMO, this can be done in a follow-up by someone else as well.

TK_TraversUnlessSpelledInSource would not only have performance implications, but also niche correctness improvements:

struct IntSpan {
    IntSpan(int*);
    IntSpan subspan(int, int);
};

template <typename T>
void test_instantiations(T val) {
-    val.subspan(0, 5);
+    val.first(5); // `IntSpan` does not have `first`
}

void test_intantiations() {
    int array[] = {0, 1, 2, 3, 4};
    auto sp = std::span<int>{array, 5};
    test_instantiations(sp);
    IntSpan intsp = array;
    test_instantiations(intsp);
}

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp

hjanuschka · 2024-12-20T21:01:01Z

feedback addressed, you were right about the template case, it is really broken, adding:

  std::optional<TraversalKind> getCheckTraversalKind() const override {
    return TK_IgnoreUnlessSpelledInSource;
  }

fixes it, but now templates seem to be skipped completely. and existing tests are not "passing" anymore:

template <typename T>
void testTemplate() {
  T arr[] = {1, 2, 3, 4, 5};
  std::span<T> s(arr, 5);

  auto sub1 = s.subspan(0, 3);
  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()'
  // CHECK-FIXES: auto sub1 = s.first(3);

  auto sub2 = s.subspan(s.size() - 2);
  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()'
  // CHECK-FIXES: auto sub2 = s.last(2);

  __SIZE_TYPE__ n = 2;
  auto sub3 = s.subspan(0, n);
  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()'
  // CHECK-FIXES: auto sub3 = s.first(n);

  auto sub4 = s.subspan(1, 2);  // No warning
  auto sub5 = s.subspan(2);     // No warning

  auto complex = s.subspan(0 + (s.size() - 2), 3);  // No warning

  auto complex2 = s.subspan(100 + (s.size() - 2));  // No warning
}

do you have any ideas?

denzor200 · 2024-12-19T11:27:51Z

clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp

+  auto good2 = s2.subspan(std::ranges::size(s2) - 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer 'span::last()' over 'subspan()'
+  // CHECK-FIXES: auto good2 = s2.last(3);
+}


please add test_span_of_bytes:

std::byte arr[] = {0x1, 0x2, 0x3, 0x4, 0x5}; std::span<std::byte> s(arr, 5); // ... auto sub2 = s.subspan(s.size_bytes() - 2); // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' // CHECK-FIXES: auto sub2 = s.last(2); // ...

5chmidti · 2024-12-23T19:04:25Z

but now templates seem to be skipped completely

Template instantiations are skipped, not their definition. The matchers no longer work, because the types you are working with are no longer necessarily concrete types, but instead they can be dependent. And so can be expressions.

In addition to the matchers for non-dependent code, you need to add matchers for the dependent case. See godbolt link I had linked: https://godbolt.org/z/GrfMTxeGa
and the included matcher (example, not full matcher):

callExpr(                            // no longer a CXXMemberCallExpr
    callee(
        cxxDependentScopeMemberExpr( // the callee is now a dependent member
            hasMemberName("subspan"),
            hasObjectExpression(
                hasType(             // this part is effectively HasSpanType but for a dependent type of `std::span<T>`
                    elaboratedType(
                        namesType(
                            qualType(
                                hasCanonicalType(
                                    qualType(
                                        hasDeclaration(
                                            namedDecl(hasName("::std::span"))
                                        )
                                    )
                                )
                            )
                        )
                    )
                )
            )
        )
    )
)

hjanuschka added 2 commits November 29, 2024 10:17

[clang-tidy] Add modernize-use-span-first-last check

cb748c3

Add new check that modernizes std::span::subspan() calls to use the more expressive first() and last() member functions where applicable.

format

ec5a6b0

llvmbot added clang-tools-extra clang-tidy labels Nov 29, 2024

format

5cf1b7c

carlosgalvezp reviewed Nov 29, 2024

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseSpanFirstLastCheck.h Outdated Show resolved Hide resolved

hjanuschka added 4 commits November 29, 2024 10:36

format

b357f8c

format

42d1df2

format

04f3edc

format

2f9cc3e

hjanuschka changed the title ~~[clang-tidy] Add modernize-use-span-first-last check~~ [clang-tidy] Add readability-use-span-first-last check Nov 29, 2024

hjanuschka added 3 commits November 29, 2024 10:53

up

24690e3

up

39833d4

format

964fadc

hjanuschka added 3 commits November 29, 2024 11:21

format

79afa9e

format

31d300f

format

c7fd4d5

EugeneZelenko reviewed Nov 29, 2024

View reviewed changes

EugeneZelenko requested review from 5chmidti and PiotrZSL November 29, 2024 16:01

5chmidti requested changes Nov 30, 2024

View reviewed changes

hjanuschka and others added 3 commits December 2, 2024 21:11

Update clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck…

4a9fc6e

….cpp Co-authored-by: EugeneZelenko <[email protected]>

Update clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h

3f604e5

Co-authored-by: EugeneZelenko <[email protected]>

feed

7e2aaa4

EugeneZelenko reviewed Dec 3, 2024

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst Outdated Show resolved Hide resolved

up

8c16f74

5chmidti reviewed Dec 4, 2024

View reviewed changes

HerrCai0907 reviewed Dec 4, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp Outdated Show resolved Hide resolved

HerrCai0907 reviewed Dec 4, 2024

View reviewed changes

hjanuschka added 2 commits December 5, 2024 08:42

up

514ac50

up

86b467b

EugeneZelenko reviewed Dec 5, 2024

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst Outdated Show resolved Hide resolved

up

6ab0051

hjanuschka requested a review from 5chmidti December 5, 2024 19:07

5chmidti reviewed Dec 6, 2024

View reviewed changes

hjanuschka added 3 commits December 11, 2024 22:34

up

39da599

feedback

8ba6771

feedback

79702fb

EugeneZelenko reviewed Dec 11, 2024

View reviewed changes

clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp Outdated Show resolved Hide resolved

5chmidti reviewed Dec 11, 2024

View reviewed changes

hjanuschka added 2 commits December 14, 2024 11:33

feedback

0a1d6cb

feedback

3354841

hjanuschka commented Dec 14, 2024

View reviewed changes

hjanuschka requested a review from 5chmidti December 14, 2024 14:51

5chmidti reviewed Dec 20, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp Outdated Show resolved Hide resolved

feedback

1fc1375

denzor200 reviewed Dec 21, 2024

View reviewed changes

[clang-tidy] Add readability-use-span-first-last check #118074

Are you sure you want to change the base?

[clang-tidy] Add readability-use-span-first-last check #118074

Uh oh!

Conversation

hjanuschka commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosgalvezp commented Nov 29, 2024

Uh oh!

carlosgalvezp commented Nov 29, 2024

Uh oh!

Uh oh!

hjanuschka commented Nov 29, 2024

Uh oh!

EugeneZelenko Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

hjanuschka Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

EugeneZelenko Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pkasting commented Nov 29, 2024

Uh oh!

5chmidti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hjanuschka commented Dec 4, 2024

Uh oh!

carlosgalvezp commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HerrCai0907 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjanuschka commented Dec 5, 2024

Uh oh!

HerrCai0907 commented Dec 5, 2024

Uh oh!

hjanuschka commented Dec 5, 2024

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hjanuschka commented Nov 29, 2024 •

edited

Loading

llvmbot commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

5chmidti left a comment •

edited

Loading

carlosgalvezp commented Dec 4, 2024 •

edited

Loading

HerrCai0907 left a comment •

edited

Loading