rsmith 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
+ //
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146465/new/
https://reviews.llvm.org/D146465
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
