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