Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang-tools-extra/clang-tidy/readability/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
UppercaseLiteralSuffixCheck.cpp
UseAnyOfAllOfCheck.cpp
UseStdMinMaxCheck.cpp
UseSpanFirstLastCheck.cpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


LINK_LIBS
clangTidy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
#include "UniqueptrDeleteReleaseCheck.h"
#include "UppercaseLiteralSuffixCheck.h"
#include "UseAnyOfAllOfCheck.h"
#include "UseSpanFirstLastCheck.h"
#include "UseStdMinMaxCheck.h"

namespace clang::tidy {
Expand Down Expand Up @@ -172,6 +173,8 @@ class ReadabilityModule : public ClangTidyModule {
"readability-use-anyofallof");
CheckFactories.registerCheck<UseStdMinMaxCheck>(
"readability-use-std-min-max");
CheckFactories.registerCheck<UseSpanFirstLastCheck>(
"readability-use-span-first-last");
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
}
};

Expand Down
110 changes: 110 additions & 0 deletions clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
//===--- UseSpanFirstLastCheck.cpp - clang-tidy-----------------*- C++ -*-===//
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
//
// 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::readability {

void UseSpanFirstLastCheck::registerMatchers(MatchFinder *Finder) {
if (!getLangOpts().CPlusPlus20)
return;
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved

// 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) {
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;

// Check if this is subspan(0, n) -> first(n)
bool IsZeroOffset = false;
const Expr *OffsetE = Offset->IgnoreImpCasts();
if (const auto *IL = dyn_cast<IntegerLiteral>(OffsetE)) {
IsZeroOffset = IL->getValue() == 0;
}
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved

// Check if this is subspan(size() - n) -> last(n)
bool IsSizeMinusN = false;
const Expr *SizeMinusArg = nullptr;
if (const auto *BO = dyn_cast<BinaryOperator>(OffsetE)) {
if (BO->getOpcode() == BO_Sub) {
if (const auto *SizeCall = dyn_cast<CXXMemberCallExpr>(BO->getLHS())) {
if (SizeCall->getMethodDecl()->getName() == "size") {
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
IsSizeMinusN = true;
SizeMinusArg = BO->getRHS();
}
}
}
}

// Build replacement text
std::string Replacement;
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
if (IsZeroOffset && Count) {
// subspan(0, count) -> first(count)
auto CountStr = Lexer::getSourceText(
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
CharSourceRange::getTokenRange(Count->getSourceRange()),
Context.getSourceManager(), Context.getLangOpts());
const auto *Base =
cast<CXXMemberCallExpr>(Call)->getImplicitObjectArgument();
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
auto BaseStr = Lexer::getSourceText(
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
CharSourceRange::getTokenRange(Base->getSourceRange()),
Context.getSourceManager(), Context.getLangOpts());
Replacement = BaseStr.str() + ".first(" + CountStr.str() + ")";
} else if (IsSizeMinusN && SizeMinusArg) {
// subspan(size() - n) -> last(n)
auto ArgStr = Lexer::getSourceText(
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
CharSourceRange::getTokenRange(SizeMinusArg->getSourceRange()),
Context.getSourceManager(), Context.getLangOpts());
const auto *Base =
cast<CXXMemberCallExpr>(Call)->getImplicitObjectArgument();
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
auto BaseStr = Lexer::getSourceText(
CharSourceRange::getTokenRange(Base->getSourceRange()),
Context.getSourceManager(), Context.getLangOpts());
Replacement = BaseStr.str() + ".last(" + ArgStr.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);
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
} // namespace clang::tidy::readability
44 changes: 44 additions & 0 deletions clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
//===--- UseSpanFirstLastCheck.h - clang-tidy-------------------*- C++ -*-===//
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
//
// 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_USESPANFIRSTLASTCHECK_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESPANFIRSTLASTCHECK_H

#include "../ClangTidyCheck.h"

namespace clang::tidy::readability {

/// Suggests using clearer std::span member functions first()/last() instead of
/// equivalent subspan() calls where applicable.
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
///
/// For example:
/// \code
/// std::span<int> s = ...;
/// auto sub1 = s.subspan(0, n); // -> auto sub1 = s.first(n);
/// auto sub2 = s.subspan(s.size() - n); // -> auto sub2 = s.last(n);
/// auto sub3 = s.subspan(1, n); // not changed
/// auto sub4 = s.subspan(n); // not changed
/// \endcode
///
/// The check is only active in C++20 mode.
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;

hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
private:
void handleSubspanCall(const ast_matchers::MatchFinder::MatchResult &Result,
const CXXMemberCallExpr *Call);
};

} // namespace clang::tidy::readability

#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESPANFIRSTLASTCHECK_H
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ New checks
New check aliases
^^^^^^^^^^^^^^^^^

- New check `readability-use-span-first-last` has been added that suggests using
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
``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.
Expand Down
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
.. title:: clang-tidy - readability-use-span-first-last

readability-use-span-first-last
===============================

Checks for uses of ``std::span::subspan()`` that can be replaced with clearer
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
``first()`` or ``last()`` member functions. These dedicated methods were added
to C++20 to provide more expressive alternatives to common subspan operations.

Covered scenarios:

==================================== ==================================
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
Expression Replacement
------------------------------------ ----------------------------------
``s.subspan(0, n)`` ``s.first(n)``
``s.subspan(s.size() - n)`` ``s.last(n)``
==================================== ==================================

Non-zero offset with count (like ``subspan(1, n)``) or offset-only calls
(like ``subspan(n)``) have no clearer equivalent using ``first()`` or
``last()``, so these cases are not transformed.

This check is only active when C++20 or later is used.
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// RUN: %check_clang_tidy -std=c++20 %s readability-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(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
}
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
hjanuschka marked this conversation as resolved.
Show resolved Hide resolved
Loading