[PATCH] D147349: [C2x] Implement support for empty brace initialization (WG14 N2900 and WG14 N3011)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added a comment.

In D147349#4240847 <https://reviews.llvm.org/D147349#4240847>, @erichkeane wrote:

> So not necessary to this patch, but I suspect the non-trivially constructible types could just be solved the same way we do for zero-init of these types in fixed-size arrays, right?  Its a bit more complicated, but the IR is probably just as do-able: https://godbolt.org/z/Gqf65xr8M
> There, in the in it of %6 is the only place the length matters, since it looks like we do a 'begin/end' type emit for it.  %2 is kept as the 'current element being processed', and %6 is the 'end', and %5 is the 'begin'.
>
> That said, only a few small comments/questions.

It's possible, but I'm also not convinced extending C++'s already inscrutable initialization rules is a good idea, especially given that it relates to VLAs (which are already a bit questionable as an extension to C++ IMO).



================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1670
+    // in C++, we can safely memset the array memory to zero.
+    CGF.EmitNullInitialization(Dest.getAddress(), ExprToVisit->getType());
+    return;
----------------
erichkeane wrote:
> I'd probably prefer an assert for 'initializer is empty' here to confirm this, but else this seems fine.
I can add that assert, sure.


================
Comment at: clang/test/C/C2x/n2900_n3011_2.c:41
+void test_zero_size_array() {
+  int unknown_size[] = {};
+  // CHECK: define {{.*}} void @test_zero_size_array
----------------
erichkeane wrote:
> This is strange... why is this not an error?  What is this supposed to mean?
Heh, this one took me a few minutes to convince myself is correct. This creates a zero-length array, which we support as an extension. There's a corresponding Sema test on n2900_n3011.c:18 that shows the behavior of the extension diagnostics in this case.


================
Comment at: clang/test/C/C2x/n2900_n3011_2.c:55
+  // CHECK-NEXT: store i32 12, ptr %[[I]]
+  // CHECK-NEXT: %[[MEM0:.+]] = load i32, ptr %[[I]]
+  // CHECK-NEXT: %[[MEM1:.+]] = zext i32 %[[MEM0]] to i64
----------------
erichkeane wrote:
> These MEM# names are horrible to read :/  But the test is doing the right thing it appears.
If you have suggestions for better names, I'm happy to use them.


================
Comment at: clang/test/C/C2x/n2900_n3011_2.c:96
+void test_nested_structs() {
+  struct T t1 = { 1, {} };
+  struct T t2 = { 1, { 2, {} } };
----------------
erichkeane wrote:
> A VLA of these things still works right?  Is that worth a test?
I can add a test for that, sure.


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

https://reviews.llvm.org/D147349