[PATCH] D157441: [-Wunsafe-buffer-usage] Use `Strategy` to determine whether to fix a parameter

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks amazing now. Thanks for splitting it up and fighting hard against complexity!



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2586
 #endif
         it = FixablesForAllVars.byVar.erase(it);
       } else if (Tracker.hasUnclaimedUses(it->first)) {
----------------
ziqingluo-90 wrote:
> We cannot fix reference type variable declarations for now.  
> Besides, it seems that reference type is invisible to `Expr`s.  For example,
> ```
> void foo(int * &p) {
>       p[5];  // the type of the DRE `p` is a pointer-to-int instead of a reference
> }
> ```
> So we ignore such variables explicitly here.
Ooo [[ https://xkcd.com/1053/ | you're one of today's lucky 10,000 ]]! (More like today's 0.01, I doubt there are many C++ programmers who are aware of this quirk, it's a very niche compiler gotcha.)

That's right, expressions in C++ can't have reference types. They have "value kinds" instead (lvalue, rvalue, etc) which are more fundamental (not even necessarily C++-specific). As far as I understand, from language formalism point of view, references aren't values per se; they're just names given to other values (glvalues, to be exact). There's no separate operation of "reference dereference" in C++ formalism; "undereferenced reference" expressions don't exist in the first place.

So in this example:
```lang=c++
int x = 5;
int &y = x;
int a = x + y;
```
not only the type of DeclRefExpr `y` is `int`, but also the *value* of the DeclRefExpr `y` is the glvalue named `x`. And the value of DeclRefExpr `x` is *also* the glvalue named `x`. From the point of view of C++ formalism, there's no separate location in memory named `y`. The declaration `y` simply declares a different way to refer to `x`.

This formalism is very different from pointers, where in
```lang=c++
int x = 5;
int *y = &x;
int a = x + *y;
```
you're allowed to assume that there's a separate memory location named `y` where the address of memory location named `x` is written. You can further take the address of that location and put it into a third location named `w` by writing `int **z = &y;`. You can do that indefinitely, and then roll it all back with sufficient amount of `*`s. You can also reassign your pointers, eg. `y = &a` would overwrite the data at location `y`.

You cannot do any of that with references. You cannot reassign a reference. You cannot take a reference to a reference; that's the same as a reference to the original lvalue, both simply become names given to that actual value. Similarly, the language doesn't let you take an address of the location in which the reference is stored; it doesn't even allow you to assume that such location exists.

Sure you can "convert a reference to a pointer" with the help of the operator `&`, but that's just taking address of an lvalue it refers to. This doesn't mean that the reference itself "is" an address, it just means that the lvalue itself is technically still just an address. References can often be implemented as pointers by the codegen, eg. reference-type fields in classes or reference-type return values are probably just pointers "under the hood". Conversely, in case of pointers the actual location for `y` and for `z` could have also been optimized away by the optimizer, causing the pointers to "act like references" "under the hood": from CodeGen perspective they're just a different way to refer to `x`. But the purpose of this formalism isn't to describe what happens under the hood; it's just to define what operations are allowed on which expressions. And for that very simple and limited purpose, references don't act like values, and expressions of reference type don't really make sense, which is what we observe in the AST.

(None of this has anything to do with the patch, I just hope this helps you approach this problem in the future.)


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

https://reviews.llvm.org/D157441