rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land.
Minor comments; otherwise LGTM. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:6256 + // Compound literals that have automatic storage duration are destroyed at + // the end of the scope. + ---------------- Probably add "in C; in C++, they're just temporaries". ================ Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) + pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + ---------------- ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall wrote: > > > > ahatanak wrote: > > > > > rjmccall wrote: > > > > > > ahatanak wrote: > > > > > > > rjmccall wrote: > > > > > > > > rjmccall wrote: > > > > > > > > > Unfortunately, the lifetime of compound literals in C is not > > > > > > > > > this simple; they're like blocks in that they're destroyed at > > > > > > > > > the end of the enclosing scope rather than at the end of the > > > > > > > > > current statement. (The cleanup here will be popped at the > > > > > > > > > end of the full-expression if we've entered an > > > > > > > > > `ExprWithCleanups`.) And the l-value case is exactly the case > > > > > > > > > where this matters. > > > > > > > > > > > > > > > > > > I think you need to do something like what we do with blocks, > > > > > > > > > where we record all the blocks in the full-expression on the > > > > > > > > > `ExprWithCleanups` so that we can push an inactive cleanup > > > > > > > > > for them and then activate it when we emit the block. > > > > > > > > Can we make the check here something like (1) this is a > > > > > > > > block-scope compound literal and (2) it has a > > > > > > > > non-trivially-destructed type (of any kind)? That way we're > > > > > > > > not conflating two potentially unrelated elements, the lifetime > > > > > > > > of the object and the kinds of types that can be constructed by > > > > > > > > the literal. > > > > > > > > > > > > > > > > Oh, actually, there's a concrete reason to do this: C99 > > > > > > > > compound literals are not required to have struct type; they > > > > > > > > can have any object type, including arrays but also scalars. > > > > > > > > So we could, even without non-trivial C structs, have a > > > > > > > > block-scope compound of type `__strong id[]`; I guess we've > > > > > > > > always just gotten this wrong. Please add tests for this case. > > > > > > > > :) > > > > > > > There is a check `E->isFileScope()` above this. Is that > > > > > > > sufficient to check for block-scoped compound literals? > > > > > > That plus the C/C++ difference; compound literals in C++ are just > > > > > > temporaries. > > > > > I haven't been able to come up with a piece of C++ code that executes > > > > > `EmitCompoundLiteralLValue`. The following code gets rejected because > > > > > you can't take the address of a temporary object in C++: > > > > > > > > > > ``` > > > > > StrongSmall *p = &(StrongSmall){ 1, 0 }; > > > > > ``` > > > > > > > > > > If a bind a reference to it, > > > > > `AggExprEmitter::VisitCompoundLiteralExpr` is called. > > > > That makes sense; they're not gl-values in C++. It would be reasonable > > > > to assert that. But the C++ point does apply elsewhere. > > > It turns out this function is called in C++ when the compound literal is > > > a vector type, so I've just added a check for C++ instead of an assert. > > Really? Is the expression actually an l-value in this case somehow? > I see this function being called when > `ScalarExprEmitter::VisitCompoundLiteralExpr` calls `EmitLoadOfLValue`. Okay. As a general rule, nothing should be calling `EmitLValue` on an expression that isn't actually an l-value. It's a little more reasonable here because it's more like a helper routine. If this is the cleanest way to handle this in C++, it's okay, but please leave a comment explaining that here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64464/new/ https://reviews.llvm.org/D64464 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits