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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am going to do another pass but this is my initial set of comments.



================
Comment at: clang/include/clang-c/Index.h:1537
+   * initializer.
+   *
+   */
----------------
nit: extra line


================
Comment at: clang/include/clang/AST/ExprCXX.h:4728-4729
+/// void foo() {
+///   A a1(0);        // legal in C++20
+///   A a2(1.5, 1.0); // legal in C++20
+/// }
----------------



================
Comment at: clang/include/clang/AST/ExprCXX.h:4741-4742
+/// void foo() {
+///   A a(1.5); // legal in C++20
+///   A b{1.5}; // illegal !
+/// }
----------------



================
Comment at: clang/include/clang/AST/ExprCXX.h:4779
+  const ArrayRef<Expr *> getInitExprs() const {
+    return {getTrailingObjects<Expr *>(), NumExprs};
+  }
----------------
Should we prefer `makeArrayRef`?


================
Comment at: clang/lib/AST/Expr.cpp:3699
+    // FIXME: unimplemented
+    llvm_unreachable("unimplemented");
   }
----------------
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`.


================
Comment at: clang/lib/AST/ExprClassification.cpp:446
+
+  case Expr::CXXParenListInitExprClass:
+    // FIXME: unimplemented
----------------
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 


================
Comment at: clang/lib/AST/StmtPrinter.cpp:2459
+void StmtPrinter::VisitCXXParenListInitExpr(CXXParenListInitExpr *Node) {
+  // FIXME: unimplemented
+  llvm_unreachable("unimplemented");
----------------
Assuming we go w/ `InitListExpr` as a base class then you should do something similar to ` StmtPrinter::VisitInitListExpr(...)`  except with parens.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1618
+            Dest.getAddress(), RD, BaseRD,
+            /*isBaseVirtual*/ false);
+        AggValueSlot AggSlot = AggValueSlot::forAddr(
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:5263
 
+static void TryOrBuildParenListInitialization(
+    Sema &S, const InitializedEntity &Entity, const InitializationKind &Kind,
----------------
I feel like this function could use a lot of comments, I found it hard to follow the flow of logic and I think I get it mostly now but comments would have helped a lot.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5273
+    for (InitializedEntity SubEntity : Range) {
+      if (Index == 1 && Entity.getType()->isUnionType())
+        // Unions should only have one initializer expression.
----------------
Or some other clarifying comment.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5310
+      InitializationKind SubKind = InitializationKind::CreateForInit(
+          E->getExprLoc(), /*isDirectInit*/ false, E);
+      InitializationSequence Seq(S, SubEntity, SubKind, E);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531