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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

I added a few more comments, mostly nitpicks



================
Comment at: clang/lib/AST/ExprConstant.cpp:9952-9953
+    const FieldDecl *Field;
+    if (isa<InitListExpr>(E)) {
+      Field = cast<InitListExpr>(E)->getInitializedFieldInUnion();
+    } else if (isa<CXXParenListInitExpr>(E)) {
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:9956
+      assert(InitExprs.size() == 1 &&
+             "Number of expressions in C++ paren list is not 1");
+
----------------
Maybe that message could be a bit more clear.

"Union should have exactly 1 initializer in in C++ paren list" or something like that.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1595
 
+void AggExprEmitter::VisitCXXParenListInitExpr(CXXParenListInitExpr *E) {
+  ArrayRef<Expr *> InitExprs = E->getInitExprs();
----------------
There are some overlaps with `VisitInitListExpr`, I wonder if we should try to factor out some of the code, especially the base class initialization


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1600
+  if (const ArrayType *AT = dyn_cast<ArrayType>(E->getType())) {
+    for (auto Pair : llvm::enumerate(InitExprs)) {
+      // Initialization for every element of the array.
----------------
Maybe it would be cleaner to use a structured binding here.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1616
+        const CXXRecordDecl *BaseRD = Base.getType()->getAsCXXRecordDecl();
+        Address V = CGF.GetAddressOfDirectBaseInCompleteClass(
+            Dest.getAddress(), RD, BaseRD,
----------------
Should we assert that the base is not virtual?


================
Comment at: clang/lib/Sema/SemaInit.cpp:5281
+
+      // Unnamed bitfields should not be initialized at all,either with an arg
+      // or by default.
----------------



================
Comment at: clang/lib/Sema/TreeTransform.h:13930
+                     InitExprs.size(), true, TransformedInits,
+                     &ArgumentChanged))
+    return ExprError();
----------------
`ArgumentChanged` is not used, and is defaulted by the function, so you should be able to remove it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531