[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

[Ziqing Luo via Phabricator via cfe-commits]

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 information between them}} \
+   expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to 'std::span' to propagate bounds information between them}} \
+   expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to 'std::span' to propagate bounds information between them}}
----------------
ziqingluo-90 wrote:
> NoQ wrote:
> > QoL thing: //"to propagate bounds information between them"// isn't the correct reason in this case. Bounds information doesn't propagate between `p` and `q` and `a`. Each of them carries its own, completely unrelated bounds information.
> > 
> > We need to come up with a different reason. Or it might be better to combine the warnings into one warning here, so that we didn't need a reason:
> > ```
> > warning: 'p', 'q' and 'a' are unsafe pointers used for buffer access
> > note: change type of 'p', 'q' and 'a' to 'std::span' to preserve bounds information
> > ```
> > 
> > This gets a bit worse when parameters are implicated indirectly, but it still mostly works fine, for example:
> > ```
> > void foo(int *p, int *q) {
> >   int *p2 = p;
> >   p2[5] = 7;
> >   int *q2 = q;
> >   q2[5] = 7;
> > }
> > ```
> > would produce:
> > ```
> > warning: 'p2' and 'q2' are unsafe pointers used for buffer access
> > note: change type of 'p2' and 'q2' to 'std::span' to preserve bounds information, and
> >       change 'p' and 'q' to 'std::span' to propagate bounds information between them
> > ```
> > 
> > ---
> > 
> > In any case, this shows that the relationship between parameters (grouped together simply for being parameters) is very different from the relationship within groups of variables implicated by assignments (even if two or more of them are parameters). All the way down to the warning/note printer, we probably need to continue giving the parameter relationship a special treatment.
> This is a good argument!  An edge from `p` to `q` in an assignment graph stands for not only that fixing `p` implicates fixing `q` but also that the bounds information is propagated from `q` to `p`.   In this sense, the way I connect parameters is inappropriate.
> 
> But my concern is that although treating parameters specially could improve the quality of the diagnostic messages, the code may be more complex and less re-used.
So for such an example
```
int f(int *p, int *q) {
int * a = p;
int * b = q;

a[5] = 0;
b[5] = 0;
}
```
We will fix all four variables `a, b, p, q` together.  But bounds information propagation only happens between `a` and `p`,  and `b` and `q`, respectively. 
Then what should the diagnostic message be?  How about something like
```
change type of 'a' to 'std::span' to preserve bounds information, and change 'p' to propagate bounds information between them. 
To ensure parameters are changed together, also
change type of 'b' to 'std::span' to preserve bounds information, and change 'q' to propagate bounds information between them;
...
```
for variable `a`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153059/new/

https://reviews.llvm.org/D153059