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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
