ahatanak added inline comments.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:677
+ E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct)
+ Cleanup.setExprNeedsCleanups(true);
+
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > ahatanak wrote:
> > > > > > rjmccall wrote:
> > > > > > > Why only when the l-value is volatile?
> > > > > > I was trying to avoid emitting `ExprWithCleanups` when it wasn't
> > > > > > needed and it seemed that it wasn't needed for non-volatile types.
> > > > > > I'm not sure it's important, so I've removed the check for
> > > > > > volatile. Also, `ExprWithCleanups` shouldn't' be emitted when this
> > > > > > is in file scope, so I fixed that too.
> > > > > Hmm, not sure about this file-scope thing, since the combination of
> > > > > C++ dynamic initializers and statement-expressions means we can have
> > > > > pretty unrestricted code there.
> > > > I should have explained why this was needed, but I wanted to prevent
> > > > emitting `ExprWithCleanups` in the following example:
> > > >
> > > > ```
> > > > struct A {
> > > > id f0;
> > > > };
> > > >
> > > > typedef struct A A;
> > > >
> > > > A g = (A){ .f0 = 0 };
> > > > ```
> > > >
> > > > The l-value to r-value conversion happens here because compound
> > > > literals are l-values. Since `g` is a global of a non-trivial C struct
> > > > type, we shouldn't try to push a cleanup and destruct the object.
> > > >
> > > > We don't have to think about the C++ case since the line below checks
> > > > the type is a non-trivial C type. I didn't think about statement
> > > > expressions, but they aren't allowed in file scope, so I guess that's
> > > > not a problem either.
> > > I would hope that the constant-evaluator here might be smart enough to
> > > ignore some elidable temporaries.
> > Do you mean the constant-evaluator should evaluate initializers that are
> > `ExprWithCleanups` to constants in a case like this? It's possible to do
> > so by seeing whether the sub-expression of `ExprWithCleanups` is constant,
> > but it looks like we also have to set the
> > `CleanupInfo::CleanupsHaveSideEffects` flag to false when it's a file scope
> > expression so that `ConstExprEmitter::VisitExprWithCleanups` can
> > constant-fold the `ExprWithCleanups` initializer.
> In this case, what we're doing is eliding a "temporary" (really an object
> with wider lifetime, but if the object isn't referenceable, we can compile
> as-if the object is really temporary) and therefore bypassing the need for a
> cleanup entirely. I wouldn't expect the AST to change to reflect that this
> is possible, just the constant evaluator. Basically, it needs to look
> through `ExprWithCleanups`; we probably already peephole the
> `LValueToRValue(CompoundLiteralExpr)` combination.
>
> We do try to do constant-evaluation on local initializers as well as an
> optimization, and abstractly that should also work with these constructs.
I taught `Expr::isConstantInitializer` to look through `ExprWithCleanups` and
`ConstExprEmitter::VisitExprWithCleanups` to ignore whether the expression has
side effects. For the latter, I'm assuming we don't have to care about the side
effect of the cleanup if the need for a cleanup is elided. This change causes
`CodeGenFunction::EmitAutoVarInit` to emit the initializer as a constant too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66094/new/
https://reviews.llvm.org/D66094
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
