balazske added inline comments.
================ Comment at: clang/test/Analysis/cfg.cpp:570 } +// CHECK-LABEL: void vla_simple(int x) ---------------- aaron.ballman wrote: > NoQ wrote: > > balazske wrote: > > > balazske wrote: > > > > aaron.ballman wrote: > > > > > Can you also add tests for: > > > > > ``` > > > > > int vla_unevaluated(int x) { > > > > > // Evaluates the ++x > > > > > sizeof(int[++x]); > > > > > // Does not evaluate the ++x > > > > > _Alignof(int[++x]); > > > > > _Generic((int(*)[++x])0, default : 0); > > > > > > > > > > return x; > > > > > } > > > > > ``` > > > > > > > > > > Also, it's a bit strange to find these VLA tests in a .cpp file. VLAs > > > > > are a C++ extension and we should have some testing for them for > > > > > constructs that are C++-specific, but I would have expected many of > > > > > these tests to be in a C file instead. > > > > Could be there platforms (on buildbot?) where the VLA code (in cpp > > > > file) does not compile? Is it worth to have a test with VLA type alias > > > > then? > > > I have this output for the test above (without the new code), is it > > > correct? > > > (The `++` seems to be evaluated for the `alignof` statement too.) > > > ``` > > > int vla_unevaluated(int x) > > > [B2 (ENTRY)] > > > Succs (1): B1 > > > > > > [B1] > > > 1: x > > > 2: ++[B1.1] > > > 3: [B1.2] (ImplicitCastExpr, LValueToRValue, int) > > > 4: sizeof(int [++x]) > > > 5: x > > > 6: ++[B1.5] > > > 7: [B1.6] (ImplicitCastExpr, LValueToRValue, int) > > > 8: alignof(int [++x]) > > > 9: 0 > > > 10: x > > > 11: [B1.10] (ImplicitCastExpr, LValueToRValue, int) > > > 12: return [B1.11]; > > > Preds (1): B2 > > > Succs (1): B0 > > > > > > [B0 (EXIT)] > > > Preds (1): B1 > > > > > > ``` > > > Could be there platforms (on buildbot?) where the VLA code (in cpp file) > > > does not compile? Is it worth to have a test with VLA type alias then? > > > > That's unlikely; the problems with VLAs in C++ are IIRC about the > > bureaucracy of the Standard (i.e., they couldn't solve enough cornercases, > > like whether `T<int[x]>` and `T<int[y]>` the same template when `x == y` at > > run-time?), not about platform support. > > Could be there platforms (on buildbot?) where the VLA code (in cpp file) > > does not compile? Is it worth to have a test with VLA type alias then? > > I think the type alias test is a good idea (as are any other C++-specific > tests) because we support VLAs in C++ mode as an extension, my concern was > more that this is a C feature and so if I was hunting for tests for VLAs, I'd > look in a .c file first. It's not a huge concern, more like a style nit. > > > (The ++ seems to be evaluated for the alignof statement too.) > > That seems wrong per C17 6.5.3.4p3. It also doesn't seem to match the > behavior I'm seeing: https://godbolt.org/z/rVaGnb > There is a //cfg.cpp// test file but not a //cfg.c// so the new tests are added to the cpp file. I do not like to make a new //cfg.c// because it will contain a part of the VLA tests only because the type alias cases. It is better to have all VLA tests in the same file. The case in `vla_unevaluated` is probably wrong but an independent problem. This change would be only about adding the VLA related parts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77809/new/ https://reviews.llvm.org/D77809 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits