[PATCH] D129531: [clang][C++20] P0960R3: Allow initializing aggregates from a parenthesized list of values

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added a comment.

In D129531#3880875 <https://reviews.llvm.org/D129531#3880875>, @ayzhao wrote:

> In D129531#3873872 <https://reviews.llvm.org/D129531#3873872>, @royjacobson wrote:
>
>> Thanks for working on it! It looks really good. Please remember to update the feature test macro (__cpp_aggregate_paren_init).
>
> Done
>
>> Also, I think there's no test coverage for the ASTReader/Writer changes? I would like to see some as well.
>
> I added a precompiled header test. Currently, the ASTReader/Writer change is broken with the following error:
>
> ...
>
> I'm currently looking into this - I suspect it has to do with `CXXParenListInitExpr` storing all it's subexpressions in an `llvm::TrailingObjects` base class, but I have yet to confirm one way or another.

The PCH test is fixed now, so we now have coverage for the ASTReader/Writer changes.



================
Comment at: clang/lib/AST/Expr.cpp:3699
+    // FIXME: unimplemented
+    llvm_unreachable("unimplemented");
   }
----------------
shafik wrote:
> cor3ntin wrote:
> > You can probably just fall through, a list of expression only has side effects if the individual expressions do, which the code just below is doing.
> On Discourse, the suggestion was made to have use `InitListExpr` as a base class and in that case I believe we should do the same that that we do for `InitListExpr`.
See the response from @ilya-biryukov for why I decided against inheriting from `InitListExpr`.


================
Comment at: clang/lib/AST/ExprClassification.cpp:446
+
+  case Expr::CXXParenListInitExprClass:
+    // FIXME: unimplemented
----------------
shafik wrote:
> This looks like we should handle this similar to `InitListExpr` but I am not sure if single element if it is bound to a reference can be an lvalue holds as well here as well CC @erichkeane 
IMO; no. 

I believe the comment for `InitListExpr` refers to statements like:

```
int a;
int &b{a};
```

This doesn't apply to `CXXParenListInitExprClass` because the corresponding statement with parentheses wouldn't be parsed as a `CXXParenListInitExpr` in the first place.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:2459
+void StmtPrinter::VisitCXXParenListInitExpr(CXXParenListInitExpr *Node) {
+  // FIXME: unimplemented
+  llvm_unreachable("unimplemented");
----------------
shafik wrote:
> Assuming we go w/ `InitListExpr` as a base class then you should do something similar to ` StmtPrinter::VisitInitListExpr(...)`  except with parens.
See the response from @ilya-biryukov for why I decided against inheriting from `InitListExpr`.


================
Comment at: clang/lib/Sema/TreeTransform.h:13922
 
+template <typename Derived>
+ExprResult
----------------
This is an extra change, but `clang-format` would always make it.


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2179
+  for (unsigned I = 0; I < E->NumExprs; I++)
+    E->getTrailingObjects<Expr *>()[I] = Record.readSubExpr();
+}
----------------
ilya-biryukov wrote:
> ayzhao wrote:
> > ilya-biryukov wrote:
> > > FYI: I think this is where the crash comes from.
> > > We should allocate the trailing objects first.
> > > E.g. see how `PragmaCommentDecl::CreateDeserialized` does this.
> > This sounds like it could be the solution - thanks for looking at it!
> > 
> > Currently, I'm working on the refactor that shafik@ suggested, which was to inherit from `InitListExpr`. Hopefully, that refactor will fix this issue as `InitListExpr` stores it's subexpressions in a class member instead of using `llvm::TrailingObjects`.
> Are we trying to share code between two implementations? If so, I suggest to consider alternatives, e.g. creating a new base class and inheriting both `InitListExpr` and `CXXParentInitListExpr` to share the common code.
> 
> Inheriting `CXXParentInitListExpr` from `InitListExpr` breaks [Liskov substitution principle](https://en.wikipedia.org/wiki/Liskov_substitution_principle) and will likely to lead to bugs that are hard to chase.  `InitListExpr` is widely used and means`{}`. `CXXParenInitListExpr` is not `{}` and we do not know in advance which code is going to work for both and which code is only valid for `{}`. Reviewing all callsites that use `InitListExpr` does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).
So I figured out why this was failing. When I created the `CXXParenListInitExpr` object on the heap in `ASTReader::ReadStmtFromStream(...)` below, I just called `new` without allocating any space for the `TrailingObjects`. This is fixed now, and the PCH test passes.

> Inheriting CXXParentInitListExpr from InitListExpr breaks Liskov substitution principle and will likely to lead to bugs that are hard to chase. InitListExpr is widely used and means`{}. CXXParenInitListExpr` is not {} and we do not know in advance which code is going to work for both and which code is only valid for {}. Reviewing all callsites that use InitListExpr does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).

+1, I am convinced that inheriting from `InitListExpr` is the wrong thing to do here. Additionally, when I tried to inherit from `InitListExpr`, I found that there were a lot of things in that class that were specific to `InitListExpr` that I had to wrap with an `if` statement to make it work with `CXXParenListInitExpr`, and that the amount of code that I had to add considerably decreases readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531