ayzhao marked 2 inline comments as done.
ayzhao added inline comments.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:1582-1596
+ MultiExprArg ExprsToPass;
+ if (Exprs.size() == 1 && isa<CXXParenListInitExpr>(Exprs[0])) {
+ // C++20 [expr.static.cast]p4:
+ // An expression E can be explicitly converted to a type T...if T is an
+ // aggregate type ([dcl.init.aggr]) having a first element x and there is
+ // an implicit conversion sequence from E to the type of x
+ //
----------------
rsmith wrote:
> ayzhao wrote:
> > rsmith wrote:
> > > The input to this function should be a syntactic initializer such as a
> > > `ParenListExpr`, not an already-type-checked semantic initializer such as
> > > a `CXXParenListInitExpr`. The right place to do this unwrapping is in
> > > `TreeTransform::TransformInitializer`, where we unwrap various other
> > > kinds of semantic initializer.
> > > The right place to do this unwrapping is in
> > > `TreeTransform::TransformInitializer`, where we unwrap various other
> > > kinds of semantic initializer.
> >
> > So the `CXXParenListInitExpr` for `S` never gets passed to
> > `TreeTransform::TransformInitializer` - rather,
> > `TransformCXXParenListInitExpr(...)` calls `TransformExprs(...)` on its
> > subexpressions. If the subexpression happens to be a `CXXConstructExpr`,
> > then `TransformCXXConstructExpr` will call `TransformInitializer(...)`,
> > which is [why we saw the CXXConstructExpr getting
> > deleted](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L13030-L13036)
> >
> > Interestingly enough, if we initialize `S` with a braced init-list (i.e.
> > `S{(V&&) Vec}`), we never end up calling `TransformCXXConstructExpr(...)`
> > since [we revert to the syntatic
> > form.](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L11584-L11585).
> >
> > > The input to this function should be a syntactic initializer such as a
> > > `ParenListExpr`, not an already-type-checked semantic initializer such as
> > > a `CXXParenListInitExpr`.
> >
> > We can probably do this by returning a `ParenListExpr` in
> > `TransformCXXParenListInitExpr(...)`. This does cause some problems though.
> > If we use a `ParenListExpr`, how would we know when to bail out in line
> > 1551 (`!isa<InitListExpr>(Exprs[0]) &&
> > !isa<CXXParenListInitExpr>(Exprs[0])`)? Surely, not every instance of a
> > `ParenListExpr` would be caused by a `CXXParenListInitExpr`, and if this
> > were the case, that would be a pretty brittle assumption. Furthermore, we
> > never create a `ParenListExpr` for a corresponding `CXXParenListInitExpr`.
> >
> > As an alternative, we could probably do something similar to
> > `InitListExpr`, which has separate semantic and syntactic forms. Would that
> > work?
> >
> > Also, in general, are the `TransformFooExpr(...)` functions supposed to
> > return semantic or syntactic results? I seem to see a mix of both...
> OK, I've looked at this a bit more. It looks to me like there are problems in
> two places:
>
> 1)
> [TreeTransform::RebuildCXXParenListInitExpr](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L3889).
> When transforming the subexpressions of a `CXXParenListInitExpr`, we call
> `TransformExprs` passing `IsCall=true`, which means we call
> `TransformInitializer` on each of them, which reverts them to syntactic form.
> So we need to redo the semantic analysis for forming a `CXXParenListInitExpr`
> here, and can't just create a new `CXXParenListInitExpr` node directly.
> Probably the easiest way to handle that would be to produce a `ParenListExpr`
> here instead.
>
> 2)
> [Expr::getSubExprAsWritten](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/AST/Expr.cpp#L1978)
> ([reached via
> here](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L12070))
> is not properly handling `CXXParenListExpr` -- it should be stepping over it
> to the single argument of the expression, which was the as-written
> subexpression.
>
> But looking more into (2), it seems like there's a representational oddity
> here, too. We use a `CXXFunctionalCastExpr` wrapping a `CXXParenListExpr` to
> model both the case of `Aggregate(a)` (which is a functional cast, equivalent
> by definition to `(Aggregate)a`) and the case of `Aggregate(a, b)` (which is
> //not// a functional cast). I don't think that's necessarily a problem -- we
> also incorrectly use `CXXFunctionalCastExpr` wrapping an `InitListExpr` for
> `Aggregate{a, b}` -- but it means that `getSubExprAsWritten` will need to be
> careful to only unwrap in the case where the parenthesized aggregate
> initialization expression had exactly one element, and it's not ideal from a
> source fidelity perspective.
>
> In the long term, I think we should split `CXXTemporaryObjectExpr` into two
> -- an outer wrapper representing an explicit type construction expression of
> the form `T(...)` or `T{...}` (excluding the case `T(a)` which is a
> functional cast expression) and an inner `CXXConstructExpr` representing the
> actual constructor call. And then we can represent parenthesized aggregate
> initialization as either a `CXXTemporaryObjectExpr` wrapping a
> `CXXParenListInitExpr` (when the number of arguments is not equal to 1) or a
> `CXXFunctionalCastExpr` wrapping a `CXXParenListInitExpr` (when the number of
> arguments is equal to 1) -- and similarly we can represent the case of
> `(Aggregate)a` as a `CastExpr` wrapping a `CXXParenListInitExpr`. But for now
> I think we can leave the representation as-is and make `TreeTranform` handle
> it.
>
> (Incidentally, the name `CXXParenListInitExpr` seems confusing here -- there
> aren't necessarily any parentheses involved, and the name really ought to
> mention aggregate initialization. `CXXAggregateInitExpr` would make more
> sense to me.)
>
> > If we use a `ParenListExpr`, how would we know when to bail out in line 1551
>
> We should never see a `ParenListExpr` there (that would represent `T((a,
> b))`, not `T(a, b)`, which would result in a comma expression not a
> `ParenListExpr`). Instead, the caller of `BuildCXXTypeConstructExpr` should
> convert the `ParenListExpr` into a list of expressions passed as `Exprs`, and
> pass the locations of the parentheses as `LParenOrBraceLoc` and
> `RParenOrBraceLoc`.
>
> In particular, I think it would make sense for
> `TransformCXXFunctionalCastExpr` to detect when it's in this special case
> where the thing it's transforming isn't a functional cast expression, and
> call a different `Rebuild...` overload that takes a list of expressions
> instead of a single expression. (It would make some sense to call
> `RebuildCXXTemporaryObjectExpr` in this case.)
>
> > Also, in general, are the `TransformFooExpr(...)` functions supposed to
> > return semantic or syntactic results? I seem to see a mix of both...
>
> The intent is that `TreeTransform` on an expression removes all implicit
> nodes -- implicit conversions, semantic forms for initializers, and so on --
> and returns the node to how it would have been before it was given to `Sema`
> as a subexpression. That means that the `Sema` functionality for building an
> expression with a given set of arguments doesn't need to handle the case
> where they've already been implicitly converted in some way, which gives us
> consistent behavior between the initial parse and template instantiation.
>
> That means that when we see an expression being used as an initializer, we
> want to strip off all the purely semantic pieces and revert it back to a
> syntactic form -- that happens when transforming the initializer of a
> variable, an argument to a function, etc -- and also when we transform an
> expression that performed some kinds of conversions on its operands, we need
> to redo those conversions when transforming it.
Patch has been updated per this comment. PTAL.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146465/new/
https://reviews.llvm.org/D146465
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
