-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang-tidy] Add readability-use-span-first-last check #118074
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
Open
hjanuschka
wants to merge
33
commits into
llvm:main
Choose a base branch
from
hjanuschka:subspan
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 27 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
cb748c3
[clang-tidy] Add modernize-use-span-first-last check
hjanuschka ec5a6b0
format
hjanuschka 5cf1b7c
format
hjanuschka b357f8c
format
hjanuschka 42d1df2
format
hjanuschka 04f3edc
format
hjanuschka 2f9cc3e
format
hjanuschka 24690e3
up
hjanuschka 39833d4
up
hjanuschka 964fadc
format
hjanuschka 79afa9e
format
hjanuschka 31d300f
format
hjanuschka c7fd4d5
format
hjanuschka 4a9fc6e
Update clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck…
hjanuschka 3f604e5
Update clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h
hjanuschka 7e2aaa4
feed
hjanuschka b2e82cf
Merge branch 'subspan' of github.com:hjanuschka/llvm-project into sub…
hjanuschka c707971
up
hjanuschka cd2fce3
feedback
hjanuschka 72610ac
feedback
hjanuschka b565ef7
feedback
hjanuschka 3e9b0c6
feedback
hjanuschka 2c65d52
feedback
hjanuschka 8c16f74
up
hjanuschka 514ac50
up
hjanuschka 86b467b
up
hjanuschka 6ab0051
up
hjanuschka 39da599
up
hjanuschka 8ba6771
feedback
hjanuschka 79702fb
feedback
hjanuschka 0a1d6cb
feedback
hjanuschka 3354841
feedback
hjanuschka 1fc1375
feedback
hjanuschka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
96 changes: 96 additions & 0 deletions
96
clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| //===--- 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/AST/RecursiveASTVisitor.h" | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| #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) { | ||
| const auto HasSpanType = | ||
| hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration( | ||
| classTemplateSpecializationDecl(hasName("::std::span")))))); | ||
|
|
||
| // Match span.subspan(0, n) -> first(n) | ||
| Finder->addMatcher( | ||
| cxxMemberCallExpr( | ||
| callee(memberExpr(hasDeclaration(cxxMethodDecl(hasName("subspan"))))), | ||
| on(expr(HasSpanType).bind("span_object")), | ||
| hasArgument(0, integerLiteral(equals(0))), | ||
| hasArgument(1, expr().bind("count")), argumentCountIs(2)) | ||
| .bind("first_subspan"), | ||
| this); | ||
|
|
||
| // Match span.subspan(size() - n) or span.subspan(std::ranges::size(span) - n) | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // -> last(n) | ||
| const auto SizeCall = anyOf( | ||
| cxxMemberCallExpr( | ||
| callee(memberExpr(hasDeclaration(cxxMethodDecl(hasName("size")))))), | ||
| callExpr(callee( | ||
| functionDecl(hasAnyName("::std::size", "::std::ranges::size"))))); | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| Finder->addMatcher( | ||
| cxxMemberCallExpr( | ||
| callee(memberExpr(hasDeclaration(cxxMethodDecl(hasName("subspan"))))), | ||
| on(expr(HasSpanType).bind("span_object")), | ||
| hasArgument(0, binaryOperator(hasOperatorName("-"), hasLHS(SizeCall), | ||
| hasRHS(expr().bind("count")))), | ||
| argumentCountIs(1)) | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .bind("last_subspan"), | ||
| this); | ||
| } | ||
|
|
||
| void UseSpanFirstLastCheck::check(const MatchFinder::MatchResult &Result) { | ||
| const auto *SpanObj = Result.Nodes.getNodeAs<Expr>("span_object"); | ||
| if (!SpanObj) | ||
| return; | ||
|
|
||
| StringRef SpanText = Lexer::getSourceText( | ||
| CharSourceRange::getTokenRange(SpanObj->getSourceRange()), | ||
| *Result.SourceManager, Result.Context->getLangOpts()); | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (const auto *FirstCall = | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Result.Nodes.getNodeAs<CXXMemberCallExpr>("first_subspan")) { | ||
| const auto *Count = Result.Nodes.getNodeAs<Expr>("count"); | ||
| assert(Count && "Count expression must exist due to AST matcher"); | ||
|
|
||
| StringRef CountText = Lexer::getSourceText( | ||
| CharSourceRange::getTokenRange(Count->getSourceRange()), | ||
| *Result.SourceManager, Result.Context->getLangOpts()); | ||
|
|
||
| std::string Replacement = | ||
| (Twine(SpanText) + ".first(" + CountText + ")").str(); | ||
|
|
||
| diag(FirstCall->getBeginLoc(), "prefer 'span::first()' over 'subspan()'") | ||
| << FixItHint::CreateReplacement(FirstCall->getSourceRange(), | ||
| Replacement); | ||
| } | ||
hjanuschka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (const auto *LastCall = | ||
| Result.Nodes.getNodeAs<CXXMemberCallExpr>("last_subspan")) { | ||
| const auto *Count = Result.Nodes.getNodeAs<Expr>("count"); | ||
| assert(Count && "Count expression must exist due to AST matcher"); | ||
|
|
||
| StringRef CountText = Lexer::getSourceText( | ||
| CharSourceRange::getTokenRange(Count->getSourceRange()), | ||
| *Result.SourceManager, Result.Context->getLangOpts()); | ||
|
|
||
| std::string Replacement = SpanText.str() + ".last(" + CountText.str() + ")"; | ||
hjanuschka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| diag(LastCall->getBeginLoc(), "prefer 'span::last()' over 'subspan()'") | ||
| << FixItHint::CreateReplacement(LastCall->getSourceRange(), | ||
| Replacement); | ||
| } | ||
| } | ||
| } // namespace clang::tidy::readability | ||
43 changes: 43 additions & 0 deletions
43
clang-tools-extra/clang-tidy/readability/UseSpanFirstLastCheck.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| //===--- 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_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. | ||
| /// | ||
| /// 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; | ||
| bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { | ||
| return LangOpts.CPlusPlus20; | ||
| } | ||
| }; | ||
|
|
||
| } // namespace clang::tidy::readability | ||
|
|
||
| #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESPANFIRSTLASTCHECK_H |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
clang-tools-extra/docs/clang-tidy/checks/readability/use-span-first-last.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| .. title:: clang-tidy - readability-use-span-first-last | ||
|
|
||
| readability-use-span-first-last | ||
| =============================== | ||
|
|
||
| Suggests using ``std::span::first()`` and ``std::span::last()`` member functions | ||
| instead of equivalent ``subspan()``. These dedicated methods were added to C++20 | ||
| to provide more expressive alternatives to common subspan operations. | ||
|
|
||
| Covered scenarios: | ||
|
|
||
| =========================== ============== | ||
| 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. |
122 changes: 122 additions & 0 deletions
122
clang-tools-extra/test/clang-tidy/checkers/readability/use-span-first-last.cpp
hjanuschka marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| // 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 | ||
|
|
||
| // Add here, right after the std namespace closes: | ||
| namespace std::ranges { | ||
| template<typename T> | ||
| __SIZE_TYPE__ size(const span<T>& s) { return s.size(); } | ||
| } | ||
|
|
||
| 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 | ||
|
|
||
|
|
||
| #define ZERO 0 | ||
| #define TWO 2 | ||
| #define SIZE_MINUS(s, n) s.size() - n | ||
| #define MAKE_SUBSPAN(obj, n) obj.subspan(0, n) | ||
| #define MAKE_LAST_N(obj, n) obj.subspan(obj.size() - n) | ||
|
|
||
| auto sub6 = s.subspan(SIZE_MINUS(s, 2)); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' | ||
| // CHECK-FIXES: auto sub6 = s.last(2); | ||
|
|
||
| auto sub7 = MAKE_SUBSPAN(s, 3); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: prefer 'span::first()' over 'subspan()' | ||
| // CHECK-FIXES: auto sub7 = s.first(3); | ||
|
|
||
| auto sub8 = MAKE_LAST_N(s, 2); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: prefer 'span::last()' over 'subspan()' | ||
| // CHECK-FIXES: auto sub8 = s.last(2); | ||
|
|
||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| // Test instantiation | ||
| void testInt() { | ||
| testTemplate<int>(); | ||
| } | ||
|
|
||
| void test_ranges() { | ||
| int arr[] = {1, 2, 3, 4, 5}; | ||
| std::span<int> s(arr, 5); | ||
|
|
||
| auto sub1 = s.subspan(std::ranges::size(s) - 2); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' | ||
| // CHECK-FIXES: auto sub1 = s.last(2); | ||
|
|
||
| __SIZE_TYPE__ n = 2; | ||
| auto sub2 = s.subspan(std::ranges::size(s) - n); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' | ||
| // CHECK-FIXES: auto sub2 = s.last(n); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep alphabetical order.
There was a problem hiding this comment.
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
RedundantInlineSpecifierCheckalso wrong, should i fix this in the same PR?There was a problem hiding this comment.
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.