[clang] 7c3ad9e - [-Wunsafe-buffer-usage] Fix fixits for span initialized from const size array (#81927)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 16 13:10:55 PST 2024
Author: jkorous-apple
Date: 2024-02-16T13:10:51-08:00
New Revision: 7c3ad9e72bc034ad655a7e16aa73b9864c58768b
URL: https://github.com/llvm/llvm-project/commit/7c3ad9e72bc034ad655a7e16aa73b9864c58768b
DIFF: https://github.com/llvm/llvm-project/commit/7c3ad9e72bc034ad655a7e16aa73b9864c58768b.diff
LOG: [-Wunsafe-buffer-usage] Fix fixits for span initialized from const size array (#81927)
Example:
int arr[10];
int * ptr = arr;
If ptr is unsafe and we transform it to std::span then the fixit we'd
currently provide transforms the code to:
std::span<int> ptr{arr, 10};
That's suboptimal as that repeats the size of the array in the code.
The idiomatic transformation should rely on the span constructor
that takes just the array argument and relies on template
parameter autodeduction to set the span size.
The transformed code should look like:
std::span<int> ptr = arr;
Note that it just should not change the initializer at all and that also
works for other forms of initialization like:
int * ptr {arr};
becoming:
std::span<int> ptr{arr};
This patch changes the initializer handling to the desired (empty)
fixit.
Added:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 769c6d9ebefaa5..7734e56d1376de 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2152,15 +2152,18 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
// In many cases, this function cannot figure out the actual extent `S`. It
// then will use a place holder to replace `S` to ask users to fill `S` in. The
// initializer shall be used to initialize a variable of type `std::span<T>`.
+// In some cases (e. g. constant size array) the initializer should remain
+// unchanged and the function returns empty list. In case the function can't
+// provide the right fixit it will return nullopt.
//
// FIXME: Support multi-level pointers
//
// Parameters:
// `Init` a pointer to the initializer expression
// `Ctx` a reference to the ASTContext
-static FixItList
+static std::optional<FixItList>
FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
- const StringRef UserFillPlaceHolder) {
+ const StringRef UserFillPlaceHolder) {
const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
@@ -2176,11 +2179,11 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
std::optional<SourceLocation> InitLocation =
getEndCharLoc(Init, SM, LangOpts);
if (!InitLocation)
- return {};
+ return std::nullopt;
SourceRange SR(Init->getBeginLoc(), *InitLocation);
- return {FixItHint::CreateRemoval(SR)};
+ return FixItList{FixItHint::CreateRemoval(SR)};
}
FixItList FixIts{};
@@ -2199,7 +2202,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
if (!Ext->HasSideEffects(Ctx)) {
std::optional<StringRef> ExtentString = getExprText(Ext, SM, LangOpts);
if (!ExtentString)
- return {};
+ return std::nullopt;
ExtentText = *ExtentString;
}
} else if (!CxxNew->isArray())
@@ -2208,10 +2211,10 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
ExtentText = One;
} else if (const auto *CArrTy = Ctx.getAsConstantArrayType(
Init->IgnoreImpCasts()->getType())) {
- // In cases `Init` is of an array type after stripping off implicit casts,
- // the extent is the array size. Note that if the array size is not a
- // constant, we cannot use it as the extent.
- ExtentText = getAPIntText(CArrTy->getSize());
+ // std::span has a single parameter constructor for initialization with
+ // constant size array. The size is auto-deduced as the constructor is a
+ // function template. The correct fixit is empty - no changes should happen.
+ return FixItList{};
} else {
// In cases `Init` is of the form `&Var` after stripping of implicit
// casts, where `&` is the built-in operator, the extent is 1.
@@ -2227,7 +2230,7 @@ FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
std::optional<SourceLocation> LocPassInit = getPastLoc(Init, SM, LangOpts);
if (!LocPassInit)
- return {};
+ return std::nullopt;
StrBuffer.append(", ");
StrBuffer.append(ExtentText);
@@ -2317,12 +2320,12 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
}
// Fix the initializer if it exists:
if (const Expr *Init = D->getInit()) {
- FixItList InitFixIts =
+ std::optional<FixItList> InitFixIts =
FixVarInitializerWithSpan(Init, Ctx, UserFillPlaceHolder);
- if (InitFixIts.empty())
+ if (!InitFixIts)
return {};
- FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
- std::make_move_iterator(InitFixIts.end()));
+ FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts->begin()),
+ std::make_move_iterator(InitFixIts->end()));
// If the declaration has the form `T *ident = init`, we want to replace
// `T *ident = ` with `std::span<T> ident`:
EndLocForReplacement = Init->getBeginLoc().getLocWithOffset(-1);
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
new file mode 100644
index 00000000000000..2044056cf6eea4
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN: -fsafe-buffer-usage-suggestions \
+// RUN: -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_initing_safe_ptr(unsigned idx) {
+ int buffer[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr = buffer;
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_initing_unsafe_ptr(unsigned idx) {
+ int buffer[123321123];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+ int* ptr = buffer;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<int> ptr"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
+ ptr[idx + 1] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
index 114ceaad56e451..ca5dab15a0dff4 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -52,12 +52,8 @@ void local_variable_qualifiers_specifiers() {
int a[10];
const int * p = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::span<int const> p"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:"{"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", 10}"
const int * const q = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:24}:"std::span<int const> const q"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"{"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
int tmp;
tmp = p[5];
tmp = q[5];
@@ -88,12 +84,8 @@ void local_ptr_to_array() {
int b[n]; // If the extent expression does not have a constant value, we cannot fill the extent for users...
int *p = a;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", 10}"
int *q = b;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
// No way to know if `n` is ever mutated since `int b[n];`, so no way to figure out the extent
tmp = p[5];
tmp = q[5];
More information about the cfe-commits
mailing list