[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.


================
Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);
----------------
jkorous wrote:
> NoQ wrote:
> > ziqingluo-90 wrote:
> > > steakhal wrote:
> > > > NoQ wrote:
> > > > > ziqingluo-90 wrote:
> > > > > > steakhal wrote:
> > > > > > > I would expect this test file to grow quite a bit.
> > > > > > > As such, I think we should have more self-descriptive names for these functions.
> > > > > > > 
> > > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > > Thanks for the comment.  I agree that they should have better names or at least explaining comments.
> > > > > > 
> > > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > > 
> > > > > > I make all testing expressions arguments of `foo` so that I do not have to create statements to use these expressions while avoiding irrelevant warnings.
> > > > > That's pretty cool but please note that when `foo()` is declared this way, it becomes a "C-style variadic function" - a very exotic construct that you don't normally see in code (the only practical example is the `printf`/`scanf` family of functions). So it may be good that we cover this exotic case from the start, but it may also be really bad that we don't cover the *basic* case.
> > > > > 
> > > > > C++ offers a different way to declare variadic functions: //variadic templates// (https://en.cppreference.com/w/cpp/language/parameter_pack). These are less valuable to test because they expand to AST that's very similar to the basic case, but that also allows you to cover the basic case better.
> > > > > 
> > > > > Or you can always make yourself happy with a few overloads that cover all your needs, it's not like we're worried about code duplication in tests:
> > > > > ```lang=c++
> > > > > void foo(int);
> > > > > void foo(int, int);
> > > > > void foo(int, int, int);
> > > > > void foo(int, int, int, int);
> > > > > void foo(int, int, int, int, int);
> > > > > void foo(int, int, int, int, int, int);
> > > > > ```
> > > > IMO its fine. I would probably call it `sink()` though. Ive used the same construct for the same reason in CSA tests with this name.
> > > I don't quite get what "basic case" refers to.  Could you explain it to me a little more?
> > By "basic case" I mean the ordinary, non-variadic function call with predefined set of arguments in the prototype.
> @ziqingluo-90 If the only purpose of `foo()` is to suppress warnings about unused values then you might also consider just suppressing those warnings with relevant `-Wno...` flags.
I now see that using a C variadic function could bring some unexpected behaviors such as implicit casts from lvalue reference to rvalue.   There are following patches whose tests also use `foo`.  I plan to add an NFC patch later to remove all uses of `foo`, if this sounds ok. 


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

https://reviews.llvm.org/D137379