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

Wed Oct 26 02:24:38 PDT 2022

ilya-biryukov added inline comments.

================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2179
+  for (unsigned I = 0; I < E->NumExprs; I++)
+    E->getTrailingObjects<Expr *>()[I] = Record.readSubExpr();
+}
----------------
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531