This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG33f6161d9eaa: [-Wunsafe-buffer-usage] Group parameter
fix-its (authored by ziqingluo-90).
Changed prior to commit:
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Everything looks great now, let's commit!
Again, thank you Ziqing for simplifying and splitting up the patch. It looks
like we're able to keep code complexity from exploding. That makes me feel
ziqingluo-90 updated this revision to Diff 553285.
ziqingluo-90 marked 2 inline comments as done.
ziqingluo-90 added a comment.
Address comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
NoQ added a comment.
Ok I think this looks great and almost good to go. I love how the core change
in the grouping algorithm is actually tiny and simple!
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892
+ for (unsigned i = 0; i < NumParms; i++) {
+if
ziqingluo-90 added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892
+ for (unsigned i = 0; i < NumParms; i++) {
+if (!ParmsMask[i])
+ continue;
+
NoQ wrote:
> Speaking of [[ https://reviews.llvm.org/D156762#inline-1517322
ziqingluo-90 updated this revision to Diff 547425.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
ziqingluo-90 updated this revision to Diff 547422.
ziqingluo-90 marked an inline comment as done.
ziqingluo-90 added a comment.
Address comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
NoQ added a comment.
OOo down to ±300, I love this!! I'll take a closer look tomorrow.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892
+ for (unsigned i = 0; i < NumParms; i++) {
+if (!ParmsMask[i])
+ continue;
+
Speaking of [[
ziqingluo-90 added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:37-39
+// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span p"
+// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span q"
+// CHECK:
ziqingluo-90 updated this revision to Diff 545872.
ziqingluo-90 marked 4 inline comments as done.
ziqingluo-90 added a comment.
Carved out NFC changes from the original diff.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
ziqingluo-90 added a comment.
Thanks for the suggestion of splitting this patch to smaller ones.
I have one such smaller patch ready here: https://reviews.llvm.org/D156474. It
does make it easier in describing and discussing about the changes.
Comment at:
jkorous added a comment.
I vote for splitting the patch to make both the review and any future debugging
or git archeology easier.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
___
cfe-commits
NoQ added a comment.
I just want to add to this, that the NFC part is actually insanely valuable
(despite technically not doing anything). This patch is so complex primarily
because `UnsafeBufferUsage.cpp` already has 1300 lines of code in unstructured
static functions - that's more than half
t-rasmud added a comment.
> I'm not sure it makes sense split it up now, probably depends on whether Jan
> and Rashmi have the same troubles as me ^.^
I have to admit it is a difficult patch to follow. It also has a lot of clever
ideas that deserve attention but might be lost because of the
NoQ added a comment.
Alright, I think I managed to fully chew through this patch and I think it's
beautiful and everything makes sense to me!
I have a tiny complaint though: It is very large though, 500 lines of code is
very hard to digest all at once. Because we aren't coming in with all the
t-rasmud added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:306
+ b[5] = 5; // expected-note{{used in buffer access here}}
+}
Can we have a test case with qualifiers on parameters and maybe another with a
ziqingluo-90 added a comment.
- a major change to the patch: Parameters are now grouped after the grouping
of all variables. The groups that contain parameters of the function will form
a union group.
- changes to the grouping algorithm: Groups will reside in the vector `Groups`
and being
ziqingluo-90 updated this revision to Diff 539720.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153059/new/
https://reviews.llvm.org/D153059
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
NoQ added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
+// search of connected components.
+if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
ziqingluo-90 wrote:
> NoQ wrote:
> >
t-rasmud added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:251
+}
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
Does this patch handle virtual methods? Ideally we'd like the fixed methods to
have the same
t-rasmud added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:5
+
+// FIXME: what about possible diagnostic message non-determinism?
+
I have used a workaround for non-determinism by using regular expressions
ziqingluo-90 added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
+// search of connected components.
+if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
NoQ wrote:
> ziqingluo-90 wrote:
NoQ added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
+// search of connected components.
+if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
ziqingluo-90 wrote:
> NoQ wrote:
> >
ziqingluo-90 added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
+// search of connected components.
+if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
NoQ wrote:
> ziqingluo-90 wrote:
NoQ added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2293
+// search of connected components.
+if (!ParmsNeedFix.empty()) {
+ auto First = ParmsNeedFix.begin(), Last = First;
ziqingluo-90 wrote:
> NoQ wrote:
> > What
ziqingluo-90 added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp:13-15
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds
information, and change 'q' and 'a' to 'std::span' to propagate bounds
ziqingluo-90 added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2077
+
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
FixItsForVariable[VD] =
NoQ wrote:
> There's a bug in variable naming:
NoQ added a comment.
Ok whew that's a big patch! I *think* I found a couple of bugs. In any case,
this code is quickly becoming very complicated, and the functions we're editing
are rapidly growing out of control. Chopping them up into smaller functions
with clear separation of concerns would
NoQ added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2077
+
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
FixItsForVariable[VD] =
There's a bug in variable naming: `FixablesForUnsafeVars`actually contains
NoQ added a comment.
I'll just leave this tiny bit of analysis here, that we did on the whiteboard a
while ago.
> The "intermediate" overload `void f(std::span, int *)` stays there but
> usually is not useful. If there are more than two parameters need to be
> fixed, there will be more such
30 matches
Mail list logo
