[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
ziqingluo-90 added a comment. Addressed comments and have landed this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156189/new/ https://reviews.llvm.org/D156189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ziqingluo-90 marked 4 inline comments as done.
Closed by commit rG3a67b912386e: [-Wunsafe-buffer-usage] Refactor to let local
variable fix-its and parameter… (authored by ziqingluo-90).
Changed prior to commit:
https://reviews.llvm.org/D156189?vs=544111=552142#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -80,28 +80,16 @@
);
}
-void testArraySubscriptsWithAuto(int *p, int **pp) {
+void testArraySubscriptsWithAuto() {
int a[10];
- auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
-
+ // We do not fix a declaration if the type is `auto`. Because the actual type may change later.
+ auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
foo(ap1[1]);// expected-note{{used in buffer access here}}
- auto ap2 = p; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
-
+ // In case the type is `auto *`, we know it must be a pointer. We can fix it.
+ auto * ap2 = a; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
foo(ap2[1]);// expected-note{{used in buffer access here}}
-
- auto ap3 = pp; // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
-
- foo(ap3[1][1]); // expected-note{{used in buffer access here}}
- // expected-warning@-1{{unsafe buffer access}}
-
- auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
-
- foo(ap4[1]);// expected-note{{used in buffer access here}}
}
void testUnevaluatedContext(int * p) {// no-warning
@@ -395,8 +383,9 @@
}
void testMultiLineDeclStmt(int * p) {
- auto
+ int
+ *
ap1 = p; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
@@ -15,7 +15,7 @@
int bar(int *ptr);
void uneval_context_fix_pointer_dereference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -30,7 +30,7 @@
}
void uneval_context_fix_pointer_array_access() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -41,7 +41,7 @@
}
void uneval_context_fix_pointer_reference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -63,7 +63,7 @@
// FIXME: Emit fixits for each of the below use.
void uneval_context_fix_pointer_dereference_not_handled() {
- auto p = new int[10];
+ int* p = new int[10];
int tmp = p[5];
foo(sizeof(*p), sizeof(decltype(*p)));
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
===
---
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
NoQ accepted this revision.
NoQ added a comment.
LGTM! I have minor suggestions for comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1791-1794
// For a `VarDecl` of the form `T * var (= Init)?`, this
-// function generates a fix-it for the declaration, which re-declares `var` to
-// be of `span` type and transforms the initializer, if present, to a span
-// constructor---`span var {Init, Extent}`, where `Extent` may need the user
-// to fill in.
+// function generates fix-its that
+// 1) replaces `T * var` with `std::span var`; and
+// 2) changes `Init` accordingly to a span constructor, if it exists.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1828
+ if (!IdentText)
+return {};
+ // Fix the initializer if it exists:
Should we start adding debug notes to all these early-returns? Or are they
already covered by a catch-all debug note down the line?
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp:20-23
+ // We do not fix the following declaration. Because if the
+ // definition of `Int_ptr_t` gets changed, the fixed code becomes
+ // incorrect and may NOT be noticed.
Int_ptr_t x = new int[10];
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp:41-44
+ // We do not fix the following declaration. Because if the
+ // definition of `Int_ptr_t` gets changed, the fixed code becomes
+ // incorrect and may NOT be noticed.
auto p = new int[10];
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
ziqingluo-90 added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1824
+
+ if (!IdentText)
+return {};
t-rasmud wrote:
> When will this condition be satisfied? I just want to understand if there are
> code examples where there is no identifier text or is it that
> `getVarDeclIdentifierText` fails.
`!IdentText` if `getVarDeclIdentifierText` fails.
`getVarDeclIdentifierText` assumes that the identifier text is a single token.
It starts with the begin location of the identifier and tries to obtain the
end location via `Lexer::getLocForEndOfToken`.
So it could fail if 1) the identifier text is not a single token or 2) any
unexpected failure happens in obtaining valid source locations.
Case 2) could be rare I think since I don't know when it could happen. We just
need to assume that it is NOT always successful in manipulating source
locations.
For case 1), I originally thought that if the identifier is expanded from a
macro with parameters, e.g.,
```
#define MACRO(x) name
int * MACRO(x);
```
, `MACRO(x)` is not a single token. But it seems I was wrong---it is one token
to the `Lexer`. See my test at
`warn-unsafe-buffer-usage-fixits-local-var-span.cpp:186--206`.
In conclusion, `getVarDeclIdentifierText` could fail but it should be rare.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
ziqingluo-90 updated this revision to Diff 544111.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -80,28 +80,16 @@
);
}
-void testArraySubscriptsWithAuto(int *p, int **pp) {
+void testArraySubscriptsWithAuto() {
int a[10];
- auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
-
+ // We do not fix a declaration if the type is `auto`. Because the actual type may change later.
+ auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
foo(ap1[1]);// expected-note{{used in buffer access here}}
- auto ap2 = p; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
-
+ // In case the type is `auto *`, we know it must be a pointer. We can fix it.
+ auto * ap2 = a; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
foo(ap2[1]);// expected-note{{used in buffer access here}}
-
- auto ap3 = pp; // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
-
- foo(ap3[1][1]); // expected-note{{used in buffer access here}}
- // expected-warning@-1{{unsafe buffer access}}
-
- auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
-
- foo(ap4[1]);// expected-note{{used in buffer access here}}
}
void testUnevaluatedContext(int * p) {// no-warning
@@ -358,8 +346,9 @@
}
void testMultiLineDeclStmt(int * p) {
- auto
+ int
+ *
ap1 = p; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
@@ -15,7 +15,7 @@
int bar(int *ptr);
void uneval_context_fix_pointer_dereference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -30,7 +30,7 @@
}
void uneval_context_fix_pointer_array_access() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -41,7 +41,7 @@
}
void uneval_context_fix_pointer_reference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -63,7 +63,7 @@
// FIXME: Emit fixits for each of the below use.
void uneval_context_fix_pointer_dereference_not_handled() {
- auto p = new int[10];
+ int* p = new int[10];
int tmp = p[5];
foo(sizeof(*p), sizeof(decltype(*p)));
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
@@ -4,7 +4,7 @@
void basic_dereference() {
int tmp;
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG:
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
t-rasmud added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1824
+
+ if (!IdentText)
+return {};
When will this condition be satisfied? I just want to understand if there are
code examples where there is no identifier text or is it that
`getVarDeclIdentifierText` fails.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
ziqingluo-90 added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp:242
ptr[2] = 30;
- auto p = new int[10];
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span
p"
We no longer generate fix-its for a variable declaration where the type
specifier is just `auto`. This is to avoid that fix-its may become incorrect
when the adopting codebase changes. For example,
```
int x;
auto p =
```
If we fix the declaration of `p` to `std::span p{, 1}`, and later `int
x` is changed to `long x`, the program becomes incorrect and the programmer
may not know that the type of `p` should simply follow the type of `x`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156189/new/
https://reviews.llvm.org/D156189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156189: [-Wunsafe-buffer-usage] Refactor to let local variable fix-its and parameter fix-its share common code
ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: NoQ, jkorous, t-rasmud, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Refactor the code for local variable fix-its so that it reuses the code for
parameter fix-its, which is in general better. For example, cv-qualifiers are
supported.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156189
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -80,28 +80,16 @@
);
}
-void testArraySubscriptsWithAuto(int *p, int **pp) {
+void testArraySubscriptsWithAuto() {
int a[10];
- auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
-
+ // We do not fix a declaration if the type is `auto`. Because the actual type may change later.
+ auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
foo(ap1[1]);// expected-note{{used in buffer access here}}
- auto ap2 = p; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
-
+ // In case the type is `auto *`, we know it must be a pointer. We can fix it.
+ auto * ap2 = a; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
foo(ap2[1]);// expected-note{{used in buffer access here}}
-
- auto ap3 = pp; // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
-
- foo(ap3[1][1]); // expected-note{{used in buffer access here}}
- // expected-warning@-1{{unsafe buffer access}}
-
- auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
- expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
-
- foo(ap4[1]);// expected-note{{used in buffer access here}}
}
void testUnevaluatedContext(int * p) {// no-warning
@@ -358,8 +346,9 @@
}
void testMultiLineDeclStmt(int * p) {
- auto
+ int
+ *
ap1 = p; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
@@ -15,7 +15,7 @@
int bar(int *ptr);
void uneval_context_fix_pointer_dereference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -30,7 +30,7 @@
}
void uneval_context_fix_pointer_array_access() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -41,7 +41,7 @@
}
void uneval_context_fix_pointer_reference() {
- auto p = new int[10];
+ int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
@@ -63,7 +63,7 @@
// FIXME: Emit fixits for each of the below use.
void uneval_context_fix_pointer_dereference_not_handled() {
- auto p = new int[10];
+ int* p = new int[10];
int tmp = p[5];
foo(sizeof(*p), sizeof(decltype(*p)));
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
+++
