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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
