Add modernize-use-span linter check

[jj-marr]

This linter check recommends using std::span in function parameters over C-arrays or references to std::vector and std::array. This is because a std::span can take in arguments of C-arrays, vectors, arrays, or any contiguous data structure. The C++ Core Guidelines recommend using spans to pass non-owning views of contiguous memory.

The check triggers as long as contiguous data structure is used in a way that would be supported by std::span. For instance, if a function takes a vector, calling push_back or using the vector in another function call that needs a vector would prevent this check from triggering

The applied fixit recommends either a compile-time length defined std::span (in case of replacing a std::array) or a runtime defined length (in case of replacing std::vector).

TODO:

• Support C-arrays (needed for C++ Core Guidelines R.14 )
• Refactor cv-qualified parameters properly (this current refactors const std::vector<int>& to const std::span<const int> when it should be std::span<const int>, because std::span doesn't propagate const. I have no idea how to do this, so I would appreciate advice.
• Decide if pointers that are only accessed with an array index should be checked (this is needed for full support of C++ Core Guidelines I.13 and F.24 )
• Maybe add option for refactoring to gsl::span or custom span wrappers?
• Create test cases for all of the possible refactorings.
• Mock vector and array properly in the test cases.
• Actually write the "only refactor if operators are supported by std::span" logic

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- clang-tools-extra/clang-tidy/modernize/UseSpanCheck.cpp clang-tools-extra/clang-tidy/modernize/UseSpanCheck.h clang-tools-extra/test/clang-tidy/checkers/modernize/use-span.cpp clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index 1fba227a7..c07febf2e 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -81,8 +81,7 @@ public:
     CheckFactories.registerCheck<UseIntegerSignComparisonCheck>(
         "modernize-use-integer-sign-comparison");
     CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges");
-    CheckFactories.registerCheck<UseSpanCheck>(
-        "modernize-use-span");
+    CheckFactories.registerCheck<UseSpanCheck>("modernize-use-span");
     CheckFactories.registerCheck<UseStartsEndsWithCheck>(
         "modernize-use-starts-ends-with");
     CheckFactories.registerCheck<UseStdFormatCheck>("modernize-use-std-format");
diff --git a/clang-tools-extra/clang-tidy/modernize/UseSpanCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseSpanCheck.cpp
index 9a9f3c3fd..cc1d167d6 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseSpanCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseSpanCheck.cpp
@@ -7,10 +7,10 @@
 //===----------------------------------------------------------------------===//
 
 #include "UseSpanCheck.h"
+#include "../utils/IncludeInserter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
-#include "../utils/IncludeInserter.h"
 #include <string>
 
 using namespace clang::ast_matchers;
@@ -24,7 +24,8 @@ AST_MATCHER(QualType, isRefToVectorOrArray) {
 
   QualType PointeeType = Node->getPointeeType();
 
-  const Type *UnqualifiedType = PointeeType.getTypePtr()->getUnqualifiedDesugaredType();
+  const Type *UnqualifiedType =
+      PointeeType.getTypePtr()->getUnqualifiedDesugaredType();
   if (!UnqualifiedType || !UnqualifiedType->isRecordType())
     return false;
 
@@ -41,18 +42,17 @@ UseSpanCheck::UseSpanCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       Inserter(utils::IncludeSorter::IS_LLVM, areDiagsSelfContained()) {}
 
-void UseSpanCheck::registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+void UseSpanCheck::registerPPCallbacks(const SourceManager &SM,
+                                       Preprocessor *PP,
                                        Preprocessor *ModuleExpanderPP) {
   Inserter.registerPreprocessor(PP);
 }
 
 void UseSpanCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(
-      functionDecl(
-          forEachDescendant(
-              parmVarDecl(hasType(qualType(isRefToVectorOrArray())))
-                  .bind("param"))),
-      this);
+  Finder->addMatcher(functionDecl(forEachDescendant(
+                         parmVarDecl(hasType(qualType(isRefToVectorOrArray())))
+                             .bind("param"))),
+                     this);
 }
 
 void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
@@ -68,7 +68,8 @@ void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
   QualType PointeeType = ParamType->getPointeeType();
   bool IsConst = PointeeType.isConstQualified();
 
-  const Type *UnqualifiedType = PointeeType.getTypePtr()->getUnqualifiedDesugaredType();
+  const Type *UnqualifiedType =
+      PointeeType.getTypePtr()->getUnqualifiedDesugaredType();
   if (!UnqualifiedType || !UnqualifiedType->isRecordType())
     return;
 
@@ -85,7 +86,8 @@ void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
     return;
 
   // Get the template arguments
-  const auto *TemplateSpecRecord = cast<ClassTemplateSpecializationDecl>(Record);
+  const auto *TemplateSpecRecord =
+      cast<ClassTemplateSpecializationDecl>(Record);
   const TemplateArgumentList &Args = TemplateSpecRecord->getTemplateArgs();
   if (Args.size() < 1)
     return;
@@ -105,14 +107,16 @@ void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
   SourceRange TypeRange = TL.getSourceRange();
 
   // Create the diagnostic
-  auto Diag = diag(Param->getBeginLoc(),
-                  "parameter %0 is reference to %1; consider using std::span instead")
-      << Param
-      << (RecordName == "std::vector" ? "std::vector" : "std::array");
+  auto Diag =
+      diag(Param->getBeginLoc(),
+           "parameter %0 is reference to %1; consider using std::span instead")
+      << Param << (RecordName == "std::vector" ? "std::vector" : "std::array");
 
   // Create the fix-it hint
   std::string SpanType;
-  std::string ElementTypeWithConst = IsConst ? "const " + ElementType.getAsString() : ElementType.getAsString();
+  std::string ElementTypeWithConst = IsConst
+                                         ? "const " + ElementType.getAsString()
+                                         : ElementType.getAsString();
 
   // For std::array, we should preserve the size in the span
   if (RecordName == "std::array" && Args.size() >= 2) {
@@ -122,7 +126,8 @@ void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
       // Convert APSInt to string
       SmallString<16> SizeStr;
       Size.toString(SizeStr, 10);
-      SpanType = "std::span<" + ElementTypeWithConst + ", " + SizeStr.str().str() + ">";
+      SpanType = "std::span<" + ElementTypeWithConst + ", " +
+                 SizeStr.str().str() + ">";
     } else {
       SpanType = "std::span<" + ElementTypeWithConst + ">";
     }
@@ -135,8 +140,7 @@ void UseSpanCheck::check(const MatchFinder::MatchResult &Result) {
 
   // Add the include for <span>
   Diag << Inserter.createIncludeInsertion(
-      Result.SourceManager->getFileID(Param->getBeginLoc()),
-      "<span>");
+      Result.SourceManager->getFileID(Param->getBeginLoc()), "<span>");
 }
 
 } // namespace clang::tidy::modernize

[jj-marr]

std::span has a capability gap with a const reference to a std::vector until C++26 as std::span cannot be constructed from an initializer list. Should this be a C++26 check?

[jj-marr]

Another capability gap is that a span doesn't seem to have bounds-checking until C++26 and I'm not sure if the bounds-checking works for std::vector

[jj-marr]

Thinking more, this is not a good check at this time.

[SunBlack]

I came across it because of #156058. I think the conversion can already be useful now, but restrictions could be imposed.

std::span has a capability gap with a const reference to a std::vector until C++26 as std::span cannot be constructed from an initializer list. Should this be a C++26 check?

I guess you mean cases like this:

// before
void print(const std::vector<int>& values) {
    for (int v : values) std::cout << v << " ";
}

// after
void print(std::span<const int> values) {
    for (int v : values) std::cout << v << " ";
}

print({1, 2, 3, 4});

Could the test be made configurable for this purpose?

Option 1) By default, it only reports methods if they are within a compilation unit and thus more visible for the check. In our tests, for example, we often have vectors with enum values that could be stored in a constexpr std::array instead of a std::vector. If values are not passed as an initializer list anywhere, you could safely switch.
Option 2) It warns everywhere. You don't normally have large initializer lists, so it shouldn't matter if you first store it in a separate variable, which increases the scope but makes the method more flexible.

And in principle: You can always disable any check if you don't like it ;-)

Another capability gap is that a span doesn't seem to have bounds-checking until C++26 and I'm not sure if the bounds-checking works for std::vector

Isn't that only relevant if an at() occurs in the method that is to be adapted? std::vector::operator[] does not have the check either, so it would only be a breaking change if at() is used. In that case, the transformation proposal could also be rejected.

[jj-marr]

@SunBlack I'm still in favour of the check but I didn't see myself working on it until C++26 as it won't be usable for many people.

I would prefer Option 2 if I was still working on this. In the example you gave, the initializer list should be in a constexpr std::array to avoid using magic numbers.

Isn't that only relevant if an at() occurs in the method that is to be adapted?

This is a footgun, because I believe std::span<T, N> does bounds-checking dependent on N. Refactoring std::array<T, N> to std::span<T> could be unsafe since N would be defaulted to a "dynamic extent" and the size is no longer encoded in the type. Cpp insights not sure what the full implications of this are.