[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables #81935

jkorous-apple · 2024-02-15T22:53:22Z

Example:
int * const my_var = my_initializer;

Currently when transforming my_var to std::span the fixits:

replace "int * const my_var = " with "std::span const my_var {"
add ", SIZE}" after "my_initializer" where SIZE is either inferred or a placeholder

This patch makes that behavior less intrusive by not modifying variable cv-qualifiers and initialization syntax.
The new behavior is:

replace "int *" with "std::span"
add "{" before "my_initializer"
add ", SIZE}" after "my_initializer"

This is an improvement on its own - since we don't touch the identifier, we automatically can handle macros in them.
It also simplifies future work on initializer fixits.

llvmbot · 2024-02-15T22:53:53Z

@llvm/pr-subscribers-clang-analysis

Author: None (jkorous-apple)

Changes

depends on
#81927

Example:
int * const my_var = my_initializer;

Currently when transforming my_var to std::span the fixits:

replace "int * const my_var = " with "std::span<int> const my_var {"
add ", SIZE}" after "my_initializer" where SIZE is either inferred or a placeholder

This patch makes that behavior less intrusive by not modifying variable cv-qualifiers and initialization syntax.
The new behavior is:

replace "int *" with "std::span<int>"
add "{" before "my_initializer"
add ", SIZE}" after "my_initializer"

This is an improvement on its own - since we don't touch the identifier, we automatically can handle macros in them.
It also simplifies future work on initializer fixits.

Patch is 54.42 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81935.diff

19 Files Affected:

(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+28-33)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp (+3-3)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp (+2-2)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp (+1-1)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp (+19)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp (+1-1)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp (+3-3)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp (+103)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp (+25-33)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp (+6-6)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp (+8-8)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp (+4-4)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp (+4-4)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp (+1-1)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp (+3-3)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-fixits-test.cpp (+12-12)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init-fixits.cpp (+12-12)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-uuc-fixits.cpp (+6-6)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp (+16-16)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 769c6d9ebefaa5..b4bda316f11be5 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);
@@ -2301,37 +2304,29 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
   std::stringstream SS;
 
   SS << *SpanTyText;
-  // Append qualifiers to the type of `D`, if any:
-  if (D->getType().hasQualifiers())
-    SS << " " << D->getType().getQualifiers().getAsString();
-
-  // The end of the range of the original source that will be replaced
-  // by `std::span<T> ident`:
-  SourceLocation EndLocForReplacement = D->getEndLoc();
-  std::optional<StringRef> IdentText =
-      getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
-
-  if (!IdentText) {
-    DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
-    return {};
-  }
   // 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()));
-    // 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);
-  }
-  SS << " " << IdentText->str();
+    FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts->begin()),
+                  std::make_move_iterator(InitFixIts->end()));
+  }
+  // For declaration of the form `T * ident = init;`, we want to replace
+  // `T * ` with `std::span<T>`.
+  // We ignore CV-qualifiers so for `T * const ident;` we also want to replace
+  // just `T *` with `std::span<T>`.
+  const SourceLocation EndLocForReplacement = D->getTypeSpecEndLoc();
   if (!EndLocForReplacement.isValid()) {
     DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the end of the declaration");
     return {};
   }
+  // The only exception is that for `T *ident` we'll add a single space between
+  // "std::span<T>" and "ident".
+  if (EndLocForReplacement.getLocWithOffset(1) == getVarDeclIdentifierLoc(D))
+    SS << " ";
+
   FixIts.push_back(FixItHint::CreateReplacement(
       SourceRange(D->getBeginLoc(), EndLocForReplacement), SS.str()));
   return FixIts;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
index 5c03cc10025c68..1484f7e9d36c2a 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
@@ -5,7 +5,7 @@ void foo(int * , int *);
 
 void add_assign_test(unsigned int n, int *a, int y) {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p += 2;
@@ -13,7 +13,7 @@ void add_assign_test(unsigned int n, int *a, int y) {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:")"
   
   int *r = p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
   while (*r != 0) {
@@ -48,7 +48,7 @@ void add_assign_test(unsigned int n, int *a, int y) {
 
 int expr_test(unsigned x, int *q, int y) {
   char *p = new char[8];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<char> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<char> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 8}"
   p += (x + 1);
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
index 062701a3f879b6..9c2908d4c43150 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
@@ -52,7 +52,7 @@ void ignore_unsafe_calls(int x) {
 	   &p[x]);
 
   int * q = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
   unsafe_f((unsigned long) &q[5],
@@ -78,7 +78,7 @@ void index_is_zero() {
 
 void pointer_subtraction(int x) {
   int * p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
 
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
index ff91e32bdc1ccf..5764ffabcf4ae8 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -15,7 +15,7 @@ void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
   int buffer[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   int* ptr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"std::span<int>"
   ptr = buffer;
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   ptr[idx] = 0;
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..0b7094e246b218
--- /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]]:7}:"std::span<int>"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
+  ptr[idx + 1] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
index 82e746703d3324..7268cd5d56b7df 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
@@ -10,7 +10,7 @@
 // By testing presence of the declaration Fix-It we indirectly test presence of the trivial Fix-It for its operations.
 void test() {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p[5] = 1;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
index a4a09a0afed595..843f3f6dcb280d 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
@@ -10,7 +10,7 @@
 
 void basic() {
   int *ptr;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   *(ptr+5)=1;
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:""
 // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:9}:"["
@@ -20,7 +20,7 @@ void basic() {
 // The weird preceding semicolon ensures that we preserve that range intact.
 void char_ranges() {
   int *p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( p + 5 ) = 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
@@ -108,7 +108,7 @@ void char_ranges() {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]"
 
   int *ptrrrrrr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> ptrrrrrr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( ptrrrrrr + 123456 )= 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp
new file mode 100644
index 00000000000000..4c7b5df2cba5b0
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp
@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+int ptr(unsigned idx) {
+  int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const(unsigned idx) {
+  const int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr(unsigned idx) {
+  int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr_to_const(unsigned idx) {
+  const int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const_volatile(unsigned idx) {
+  const volatile int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr(unsigned idx) {
+  int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr_to_const_volatile(unsigned idx) {
+  const volatile int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * my_const_int_star;
+int typedef_ptr_to_const(unsigned idx) {
+  my_const_int_star ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef int * const my_int_star_const;
+int typedef_const_ptr(unsigned idx) {
+  my_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * const my_const_int_star_const;
+int typedef_const_ptr_to_const(unsigned idx) {
+  my_const_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_decltype(unsigned idx) {
+  int a;
+  decltype(a) * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<decltype(a)>"
+  a = ptr[idx];
+  return a;
+}
+
+int decltype_ptr(unsigned idx) {
+  int * p;
+  decltype(p) ptr = new int[1];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
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..68b2cef1919379 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
@@ -7,11 +7,11 @@ typedef int Int_t;
 void local_array_subscript_simple() {
   int tmp;
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   const int *q = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span<int const> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
   tmp = p[5];
@@ -24,11 +24,11 @@ void local_array_subscript_simple() {
   Int_ptr_t x = new int[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   Int_t * z = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> z"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<Int_t>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
   Int_t * w = new Int_t[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> w"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<Int_t>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
 
@@ -51,13 +51,9 @@ void local_array_subscript_auto() {
 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}"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
   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}"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
   int tmp;
   tmp = p[5];
   tmp = q[5];
@@ -68,12 +64,12 @@ void local_array_subscript_variable_extent() {
   int n = 10;
   int tmp;
   int *p = new int[n];
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", n}"
   // If the extent expression does not have a constant value, we cannot fill the extent for users...
   int *q = new int[n++];
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
   tmp = p[5];
@@ -87,13 +83,9 @@ void local_ptr_to_array() {
   int a[10];
   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}"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   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 #>}"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // 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];
@@ -102,7 +94,7 @@ void local_ptr_to_array() {
 void local_ptr_addrof_init() {
   int var;
   int * q = &var;
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK-DAG: fix...
[truncated]

llvmbot · 2024-02-15T22:53:53Z

@llvm/pr-subscribers-clang

Author: None (jkorous-apple)

Changes

depends on
#81927

Example:
int * const my_var = my_initializer;

Currently when transforming my_var to std::span the fixits:

replace "int * const my_var = " with "std::span<int> const my_var {"
add ", SIZE}" after "my_initializer" where SIZE is either inferred or a placeholder

This patch makes that behavior less intrusive by not modifying variable cv-qualifiers and initialization syntax.
The new behavior is:

replace "int *" with "std::span<int>"
add "{" before "my_initializer"
add ", SIZE}" after "my_initializer"

This is an improvement on its own - since we don't touch the identifier, we automatically can handle macros in them.
It also simplifies future work on initializer fixits.

Patch is 54.42 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81935.diff

19 Files Affected:

(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+28-33)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp (+3-3)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp (+2-2)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp (+1-1)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-inits-ptr.cpp (+19)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp (+1-1)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp (+3-3)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp (+103)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp (+25-33)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp (+6-6)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp (+8-8)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp (+4-4)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp (+4-4)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp (+1-1)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp (+3-3)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-fixits-test.cpp (+12-12)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init-fixits.cpp (+12-12)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-uuc-fixits.cpp (+6-6)
(modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp (+16-16)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 769c6d9ebefaa5..b4bda316f11be5 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);
@@ -2301,37 +2304,29 @@ static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
   std::stringstream SS;
 
   SS << *SpanTyText;
-  // Append qualifiers to the type of `D`, if any:
-  if (D->getType().hasQualifiers())
-    SS << " " << D->getType().getQualifiers().getAsString();
-
-  // The end of the range of the original source that will be replaced
-  // by `std::span<T> ident`:
-  SourceLocation EndLocForReplacement = D->getEndLoc();
-  std::optional<StringRef> IdentText =
-      getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
-
-  if (!IdentText) {
-    DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
-    return {};
-  }
   // 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()));
-    // 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);
-  }
-  SS << " " << IdentText->str();
+    FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts->begin()),
+                  std::make_move_iterator(InitFixIts->end()));
+  }
+  // For declaration of the form `T * ident = init;`, we want to replace
+  // `T * ` with `std::span<T>`.
+  // We ignore CV-qualifiers so for `T * const ident;` we also want to replace
+  // just `T *` with `std::span<T>`.
+  const SourceLocation EndLocForReplacement = D->getTypeSpecEndLoc();
   if (!EndLocForReplacement.isValid()) {
     DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the end of the declaration");
     return {};
   }
+  // The only exception is that for `T *ident` we'll add a single space between
+  // "std::span<T>" and "ident".
+  if (EndLocForReplacement.getLocWithOffset(1) == getVarDeclIdentifierLoc(D))
+    SS << " ";
+
   FixIts.push_back(FixItHint::CreateReplacement(
       SourceRange(D->getBeginLoc(), EndLocForReplacement), SS.str()));
   return FixIts;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
index 5c03cc10025c68..1484f7e9d36c2a 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
@@ -5,7 +5,7 @@ void foo(int * , int *);
 
 void add_assign_test(unsigned int n, int *a, int y) {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p += 2;
@@ -13,7 +13,7 @@ void add_assign_test(unsigned int n, int *a, int y) {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:")"
   
   int *r = p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
   while (*r != 0) {
@@ -48,7 +48,7 @@ void add_assign_test(unsigned int n, int *a, int y) {
 
 int expr_test(unsigned x, int *q, int y) {
   char *p = new char[8];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<char> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<char> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 8}"
   p += (x + 1);
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
index 062701a3f879b6..9c2908d4c43150 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
@@ -52,7 +52,7 @@ void ignore_unsafe_calls(int x) {
 	   &p[x]);
 
   int * q = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
   unsafe_f((unsigned long) &q[5],
@@ -78,7 +78,7 @@ void index_is_zero() {
 
 void pointer_subtraction(int x) {
   int * p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
 
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
index ff91e32bdc1ccf..5764ffabcf4ae8 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -15,7 +15,7 @@ void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
   int buffer[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   int* ptr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:7}:"std::span<int>"
   ptr = buffer;
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
   ptr[idx] = 0;
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..0b7094e246b218
--- /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]]:7}:"std::span<int>"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}123321123
+  ptr[idx + 1] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
index 82e746703d3324..7268cd5d56b7df 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
@@ -10,7 +10,7 @@
 // By testing presence of the declaration Fix-It we indirectly test presence of the trivial Fix-It for its operations.
 void test() {
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   p[5] = 1;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
index a4a09a0afed595..843f3f6dcb280d 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
@@ -10,7 +10,7 @@
 
 void basic() {
   int *ptr;
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   *(ptr+5)=1;
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:5}:""
 // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:9}:"["
@@ -20,7 +20,7 @@ void basic() {
 // The weird preceding semicolon ensures that we preserve that range intact.
 void char_ranges() {
   int *p;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( p + 5 ) = 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
@@ -108,7 +108,7 @@ void char_ranges() {
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:15}:"]"
 
   int *ptrrrrrr;
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> ptrrrrrr"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
 
   ;* ( ptrrrrrr + 123456 )= 1;
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:8}:""
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp
new file mode 100644
index 00000000000000..4c7b5df2cba5b0
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span-cv-qualifiers.cpp
@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+int ptr(unsigned idx) {
+  int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const(unsigned idx) {
+  const int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr(unsigned idx) {
+  int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_ptr_to_const(unsigned idx) {
+  const int * const ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_const_volatile(unsigned idx) {
+  const volatile int * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr(unsigned idx) {
+  int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int const_volatile_ptr_to_const_volatile(unsigned idx) {
+  const volatile int * const volatile ptr = new int[1];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:23}:"std::span<int const volatile>"
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * my_const_int_star;
+int typedef_ptr_to_const(unsigned idx) {
+  my_const_int_star ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef int * const my_int_star_const;
+int typedef_const_ptr(unsigned idx) {
+  my_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+typedef const int * const my_const_int_star_const;
+int typedef_const_ptr_to_const(unsigned idx) {
+  my_const_int_star_const ptr = new int[1];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
+
+int ptr_to_decltype(unsigned idx) {
+  int a;
+  decltype(a) * ptr = new int[1];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<decltype(a)>"
+  a = ptr[idx];
+  return a;
+}
+
+int decltype_ptr(unsigned idx) {
+  int * p;
+  decltype(p) ptr = new int[1];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int a;
+  a = ptr[idx];
+  return a;
+}
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..68b2cef1919379 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
@@ -7,11 +7,11 @@ typedef int Int_t;
 void local_array_subscript_simple() {
   int tmp;
   int *p = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
   const int *q = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span<int const> q"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const> "
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
   tmp = p[5];
@@ -24,11 +24,11 @@ void local_array_subscript_simple() {
   Int_ptr_t x = new int[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   Int_t * z = new int[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> z"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<Int_t>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
   Int_t * w = new Int_t[10];
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> w"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<Int_t>"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
 
@@ -51,13 +51,9 @@ void local_array_subscript_auto() {
 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}"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
   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}"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int const>"
   int tmp;
   tmp = p[5];
   tmp = q[5];
@@ -68,12 +64,12 @@ void local_array_subscript_variable_extent() {
   int n = 10;
   int tmp;
   int *p = new int[n];
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", n}"
   // If the extent expression does not have a constant value, we cannot fill the extent for users...
   int *q = new int[n++];
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
   tmp = p[5];
@@ -87,13 +83,9 @@ void local_ptr_to_array() {
   int a[10];
   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}"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   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 #>}"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int> "
   // 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];
@@ -102,7 +94,7 @@ void local_ptr_to_array() {
 void local_ptr_addrof_init() {
   int var;
   int * q = &var;
-  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:8}:"std::span<int>"
   // CHECK-DAG: fix...
[truncated]

Example: int * const my_var = my_initializer; Currently when transforming my_var to std::span the fixits: - replace "int * const my_var = " with "std::span<int> const my_var {" - add ", SIZE}" after "my_initializer" where SIZE is either inferred or a placeholder This patch makes that behavior less intrusive by not modifying variable cv-qualifiers and initialization syntax. The new behavior is: - replace "int *" with "std::span<int>" - add "{" before "my_initializer" - add ", SIZE}" after "my_initializer" This is an improvement on its own - since we don't touch the identifier, we automatically can handle macros in them. It also simplifies future work on initializer fixits.

ziqingluo-90

LGTM!

ziqingluo-90 · 2024-02-16T22:10:43Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

+  // `T * ` with `std::span<T>`.
+  // We ignore CV-qualifiers so for `T * const ident;` we also want to replace
+  // just `T *` with `std::span<T>`.
+  const SourceLocation EndLocForReplacement = D->getTypeSpecEndLoc();


This looks a correct optimization to me! I probably didn't realize the existence of getTypeSpecEndLoc() when I did this.

ziqingluo-90 · 2024-02-16T22:19:34Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

  if (!EndLocForReplacement.isValid()) {
    DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the end of the declaration");
    return {};
  }
+  // The only exception is that for `T *ident` we'll add a single space between
+  // "std::span<T>" and "ident".
+  if (EndLocForReplacement.getLocWithOffset(1) == getVarDeclIdentifierLoc(D))


In case the identifier is a macro expansion, e.g.,

#define IDENT ident T *IDENT;

, the if-condition will evaluate to false because the files of the two source locations are different.
But this is rare and the fix-it is still correct.

You're right! I added a FIXME

jkorous-apple requested review from haoNoQ, malavikasamak, ziqingluo-90 and t-rasmud February 15, 2024 22:53

llvmbot added clang Clang issues not falling into any other category clang:analysis labels Feb 15, 2024

jkorous-apple force-pushed the cxx-safe-buffers/minimize-span-vardecl-fixit-range branch from feffa63 to 19a3b10 Compare February 16, 2024 22:02

ziqingluo-90 approved these changes Feb 16, 2024

View reviewed changes

ziqingluo-90 reviewed Feb 16, 2024

View reviewed changes

[-Wunsafe-buffer-usage][NFC] Add a FIXME to fixLocalVarDeclWithSpan

0a8c867

jkorous-apple merged commit fde4b80 into llvm:main Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables #81935

[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables #81935

Uh oh!

jkorous-apple commented Feb 15, 2024 •

edited

Loading

Uh oh!

llvmbot commented Feb 15, 2024

Uh oh!

llvmbot commented Feb 15, 2024

Uh oh!

ziqingluo-90 left a comment

Uh oh!

ziqingluo-90 Feb 16, 2024

Uh oh!

ziqingluo-90 Feb 16, 2024

Uh oh!

jkorous-apple Feb 16, 2024

Uh oh!

Uh oh!

[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables #81935

[-Wunsafe-buffer-usage] Minimize fixit range for pointer variables #81935

Uh oh!

Conversation

jkorous-apple commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 15, 2024

Uh oh!

llvmbot commented Feb 15, 2024

Uh oh!

ziqingluo-90 left a comment

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkorous-apple commented Feb 15, 2024 •

edited

Loading