[clang-tidy]suggest use std::span as replacement of c array in C++20 for modernize-avoid-c-arrays
#108555
Conversation
…0 for modernize-avoid-c-arrays
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesFull diff: https://github.com/llvm/llvm-project/pull/108555.diff 7 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
index 89790ea70cf229..9fd0259ed1aacf 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
@@ -9,6 +9,7 @@
#include "AvoidCArraysCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
using namespace clang::ast_matchers;
@@ -38,6 +39,13 @@ AST_MATCHER(clang::ParmVarDecl, isArgvOfMain) {
return FD ? FD->isMain() : false;
}
+AST_MATCHER_P(clang::TypeLoc, alwaysTrue,
+ clang::ast_matchers::internal::Matcher<clang::TypeLoc>,
+ InnerMatcher) {
+ InnerMatcher.matches(Node, Finder, Builder);
+ return true;
+}
+
} // namespace
AvoidCArraysCheck::AvoidCArraysCheck(StringRef Name, ClangTidyContext *Context)
@@ -60,6 +68,7 @@ void AvoidCArraysCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
typeLoc(hasValidBeginLoc(), hasType(arrayType()),
+ alwaysTrue(hasParent(parmVarDecl().bind("param_decl"))),
unless(anyOf(hasParent(parmVarDecl(isArgvOfMain())),
hasParent(varDecl(isExternC())),
hasParent(fieldDecl(
@@ -72,11 +81,22 @@ void AvoidCArraysCheck::registerMatchers(MatchFinder *Finder) {
void AvoidCArraysCheck::check(const MatchFinder::MatchResult &Result) {
const auto *ArrayType = Result.Nodes.getNodeAs<TypeLoc>("typeloc");
-
+ bool const IsInParam =
+ Result.Nodes.getNodeAs<ParmVarDecl>("param_decl") != nullptr;
+ const bool IsVLA = ArrayType->getTypePtr()->isVariableArrayType();
+ // in function parameter, we also don't know the size of IncompleteArrayType.
+ const bool IsUnknownSize =
+ IsVLA || (ArrayType->getTypePtr()->isIncompleteArrayType() && IsInParam);
+ enum class RecommendType { Array, Vector, Span };
+ const RecommendType RecommendType =
+ (IsInParam && Result.Context->getLangOpts().CPlusPlus20)
+ ? RecommendType::Span
+ : IsUnknownSize ? RecommendType::Vector
+ : RecommendType::Array;
diag(ArrayType->getBeginLoc(),
"do not declare %select{C-style|C VLA}0 arrays, use "
- "%select{std::array<>|std::vector<>}0 instead")
- << ArrayType->getTypePtr()->isVariableArrayType();
+ "%select{std::array<>|std::vector<>|std::span<>}1 instead")
+ << IsVLA << static_cast<int>(RecommendType);
}
} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1ad8cedc902cb5..c5a2b3c36d130f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/casting-through-void>` check to suggest replacing
the offending code with ``reinterpret_cast``, to more clearly express intent.
+- Improved :doc:`modernize-avoid-c-arrays
+ <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest use std::span
+ as replacement of c array in C++20.
+
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
member function calls too.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-c++20.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-c++20.cpp
new file mode 100644
index 00000000000000..a9298245feaaeb
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-c++20.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-avoid-c-arrays %t
+
+int foo(int data[], int size) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not declare C-style arrays, use std::span<> instead
+ int f4[] = {1, 2};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-main.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-main.cpp
index 6549422f393aaa..f833a85164f333 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-main.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-main.cpp
@@ -1,7 +1,7 @@
-// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+// RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t
int not_main(int argc, char *argv[]) {
- // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::vector<> instead
int f4[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
}
@@ -11,7 +11,7 @@ int main(int argc, char *argv[]) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
auto not_main = [](int argc, char *argv[]) {
- // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: do not declare C-style arrays, use std::vector<> instead
int f6[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not declare C-style arrays, use std::array<> instead
};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-strings.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-strings.cpp
index f6d64848f9e3a0..c8277feb3958bf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-strings.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-strings.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t -- \
+// RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t -- \
// RUN: -config='{CheckOptions: { modernize-avoid-c-arrays.AllowStringArrays: true }}'
const char name[] = "name";
@@ -6,4 +6,4 @@ const char array[] = {'n', 'a', 'm', 'e', '\0'};
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
void takeCharArray(const char name[]);
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::vector<> instead [modernize-avoid-c-arrays]
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-three-arg-main.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-three-arg-main.cpp
index 22a4016f79f4da..ae2e0bc9c10035 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-three-arg-main.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-ignores-three-arg-main.cpp
@@ -1,8 +1,8 @@
-// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+// RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t
int not_main(int argc, char *argv[], char *argw[]) {
- // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::array<> instead
- // CHECK-MESSAGES: :[[@LINE-2]]:38: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::vector<> instead
+ // CHECK-MESSAGES: :[[@LINE-2]]:38: warning: do not declare C-style arrays, use std::vector<> instead
int f4[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
}
@@ -12,8 +12,8 @@ int main(int argc, char *argv[], char *argw[]) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
auto not_main = [](int argc, char *argv[], char *argw[]) {
- // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: do not declare C-style arrays, use std::array<> instead
- // CHECK-MESSAGES: :[[@LINE-2]]:46: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: do not declare C-style arrays, use std::vector<> instead
+ // CHECK-MESSAGES: :[[@LINE-2]]:46: warning: do not declare C-style arrays, use std::vector<> instead
int f6[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not declare C-style arrays, use std::array<> instead
};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
index ce99f0821b2230..15fc4d4954c867 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+// RUN: %check_clang_tidy -std=c++17 %s modernize-avoid-c-arrays %t
int a[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
@@ -91,4 +91,4 @@ const char name[] = "Some string";
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
void takeCharArray(const char name[]);
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::vector<> instead [modernize-avoid-c-arrays]
|
|
I'm not sure I understand this change. |
| // RUN: -config='{CheckOptions: { modernize-avoid-c-arrays.AllowStringArrays: true }}' | ||
|
|
||
| const char name[] = "name"; | ||
| const char array[] = {'n', 'a', 'm', 'e', '\0'}; | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays] | ||
|
|
||
| void takeCharArray(const char name[]); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays] | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::vector<> instead [modernize-avoid-c-arrays] |
There was a problem hiding this comment.
The check should not advice this, because std::vector allocates dynamic memory, which C array does not. std::array is the modern equivalent.
There was a problem hiding this comment.
Actually not. How can you replace void takeCharArray(const char name[]); with std::array if you don't know the array size. In C++20, we can use std::span but for C++11, the ref of std::vector or maybe std::string (but I think this check should not introduce std::string) is the only choice.
There was a problem hiding this comment.
When we have T[], the size is most likely not evident, even for the developer that has semantic knowledge of their code that may not be the case, so std::array will be unlikely to be helpful. I'd suggest that the diagnostic suggest to users to use std::vector or std::span (as in provide both options in the message) in case the array is variable, and std::array otherwise. Maybe even mentioning std::array in the variable array case.
In the case of VLAs I agree on proposing to use std::vector, because it is known that the length is variable, even if the original array may have lived on the stack this makes sense to do IMO.
In the case of incomplete array types, we can't know if the original array/buffer was statically sized and or dynamically allocated, and therefore makes suggesting std::vector problematic, at least in some cases. Using std::span would be the appropriate facility in this case, but that only applies to >=C++20. Because std::vector may not be the desired solution to the issue, the check could have an option that enables the proposal of using std::vector, and also supply an option to suggest custom pre-C++20 wrappers (e.g., gsl::span). The suggestion of std::vector could be done as well, though std::array should be suggested as well in that case due to the uncertainty of stack vs heap allocation as well as statically sized vs variable sized.
Mabye the wording of the diagnostic could also be less in the direction of: this is the solution, and instead be reworded towards a suggestion: you could use _ [or _] instead.
There was a problem hiding this comment.
Ok, I see what you mean now. While this solution is fine, I think it's probably easiest to let users specify what they should use via configuration. Most likely everyone has their own backports/coding guidelines to adhere to, and it'd be more valuable for them to be able to specify exactly what should be used.
There was a problem hiding this comment.
(Doesn't need to be done in this patch)
The C-Array in parameter also does not own memory. It is equivalent to pointer. |
| // RUN: -config='{CheckOptions: { modernize-avoid-c-arrays.AllowStringArrays: true }}' | ||
|
|
||
| const char name[] = "name"; | ||
| const char array[] = {'n', 'a', 'm', 'e', '\0'}; | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays] | ||
|
|
||
| void takeCharArray(const char name[]); | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays] | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: do not declare C-style arrays, use std::vector<> instead [modernize-avoid-c-arrays] |
There was a problem hiding this comment.
When we have T[], the size is most likely not evident, even for the developer that has semantic knowledge of their code that may not be the case, so std::array will be unlikely to be helpful. I'd suggest that the diagnostic suggest to users to use std::vector or std::span (as in provide both options in the message) in case the array is variable, and std::array otherwise. Maybe even mentioning std::array in the variable array case.
In the case of VLAs I agree on proposing to use std::vector, because it is known that the length is variable, even if the original array may have lived on the stack this makes sense to do IMO.
In the case of incomplete array types, we can't know if the original array/buffer was statically sized and or dynamically allocated, and therefore makes suggesting std::vector problematic, at least in some cases. Using std::span would be the appropriate facility in this case, but that only applies to >=C++20. Because std::vector may not be the desired solution to the issue, the check could have an option that enables the proposal of using std::vector, and also supply an option to suggest custom pre-C++20 wrappers (e.g., gsl::span). The suggestion of std::vector could be done as well, though std::array should be suggested as well in that case due to the uncertainty of stack vs heap allocation as well as statically sized vs variable sized.
Mabye the wording of the diagnostic could also be less in the direction of: this is the solution, and instead be reworded towards a suggestion: you could use _ [or _] instead.
HerrCai0907
requested review from
5chmidti,
EugeneZelenko and
carlosgalvezp
| However, fix-it are potentially dangerous in header files and are therefore not | ||
| emitted right now. | ||
|
|
||
| .. code:: c++ | ||
|
|
||
| int a[] = {1, 2}; // warning: do not declare C-style arrays, use std::array<> instead | ||
| int a[] = {1, 2}; // warning: do not declare C-style arrays, use 'std::array<>' instead |
There was a problem hiding this comment.
Nit: is adding the single quotes needed for this patch? It introduces quite some review noise. It'd be preferable to apply this as a separate (NFC?) patch.
HerrCai0907
force-pushed
the
opt-avoid-c-array-error-message
branch
from
14e526d to
aa477f5
Compare
…0 for modernize-avoid-c-arrays (llvm#108555) The incompleted C-Array in parameter does not own memory. Instead, It is equivalent to pointer. So we should not use `std::array` as recommended modern C++ replacement, but use `std::vector` and `std::span` in C++20
#109068) As discussion in #108555 (comment), split quotation mark change in a new NFC PR. It is more readable to use `'std::array'` than `std::array<>`
…0 for modernize-avoid-c-arrays (llvm#108555) The incompleted C-Array in parameter does not own memory. Instead, It is equivalent to pointer. So we should not use `std::array` as recommended modern C++ replacement, but use `std::vector` and `std::span` in C++20
llvm#109068) As discussion in llvm#108555 (comment), split quotation mark change in a new NFC PR. It is more readable to use `'std::array'` than `std::array<>`
The incompleted C-Array in parameter does not own memory. Instead, It is equivalent to pointer.
So we should not use
std::arrayas recommended modern C++ replacement, but usestd::vectorandstd::spanin C++20