ahatanak marked an inline comment as done.
ahatanak added inline comments.
================
Comment at: clang/lib/CodeGen/CGBlocks.cpp:869
+ if (auto *BD = C.dyn_cast<BlockDecl *>())
+ enterBlockScope(*this, BD);
}
----------------
rjmccall wrote:
> I wonder if we could just switch blocks to the same thing.
I think we can, but I haven't tried. We can fix it in a separate patch.
================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:664
+ // Block-scope compound literals are destroyed at the end of the enclosing
+ // scope in C.
+ bool Destruct =
----------------
I fixed the way `IsExternallyDestructed` is used based on the comment in the
other review.
================
Comment at: clang/lib/Serialization/ASTWriterStmt.cpp:1729
+ Record.push_back(serialization::COK_CompoundLiteral);
+ Record.AddStmt(CLE);
+ }
----------------
rjmccall wrote:
> Will this just serialize a second copy of the compound literal expression?
This should emit a `serialization::STMT_REF_PTR` record, which is a reference
to the previously serialized compound literal (see `ASTWriter::WriteSubStmt`).
================
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:
> > > > > > > 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`.
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
