ahatanak marked 3 inline comments as done.
ahatanak added inline comments.
================
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());
+
----------------
rjmccall wrote:
> 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.
I added a comment in `ScalarExprEmitter::VisitCompoundLiteralExpr`.
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
