[PATCH] D146465: [clang] Fix 2 bugs with parenthesized aggregate initialization

[Alan Zhao via Phabricator via cfe-commits]

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:
> 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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146465/new/

https://reviews.llvm.org/D146465