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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks!

I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions:

- `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code duplication is substantial and I think sharing the common implementation is warranted. E.g. if some code would want to change something with `ArrayFiller` or make a use of it, the work will probably need to be duplicated. To reiterate what I mentioned earlier, I believe deriving `CXXParenInitListExpr` from `InitListExpr` is not a very good option, but we might explore other possibilities: a base class or factoring out common code in a separate struct. I believe this could be done with a follow-up change, though, as the amount of changes required does not seem too big, I would be happy with first landing this patch and then cleaning up with a follow-up change.
- Did we explore the possibility of modelling this differently and somehow avoiding making `CXXParenInitListExpr` an expression. This seems to borrow from `InitListExpr`, but maybe for the wrong reasons.  Braced initializers can actually occur in expressions positions, e.g. `int a = {123}`. However, `CXXParenInistListExpr` cannot occur in expression positions outside initializations, e.g. having `CXXFunctionalCastExpr` for `aggregate_struct(123)` feels somewhat wrong to me and seems to lead to confusing error messages too. Maybe we should model it as a new expression that contains both the type and the argument list? I.e. more similar to `CXXConstructExpr`?



================
Comment at: clang/lib/Sema/SemaInit.cpp:5455
+      // All of the initialized entities are explicitly initialized by
+      // expressionse in the CXXParenListInitExpr. The semantic form is the
+      // same as the syntatic form
----------------
NIT: s/expressionse/expressions


================
Comment at: clang/lib/Sema/SemaInit.cpp:9803
+    TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+                                      /*VerifyOnly=*/false);
   }
----------------
NIT: add break 


================
Comment at: clang/lib/Sema/TreeTransform.h:14040
+  ArrayRef<Expr *> InitExprs = E->getInitExprs();
+  if (TransformExprs(reinterpret_cast<Expr *const *>(InitExprs.data()),
+                     InitExprs.size(), true, TransformedInits))
----------------
`reinterpret_cast` seems redundant here or am I missing something?


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2172
+  VisitExpr(E);
+  unsigned expectedNumExprs = Record.readInt();
+  assert(E->NumExprs == expectedNumExprs &&
----------------
NIT: `ExpectedNumExprs`



================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2181
+    E->getTrailingObjects<Expr *>()[I] = Record.readSubExpr();
+  E->updateDependence();
+
----------------
NIT: maybe add a comment that dependence does not depend on filler or syntactic form. This might be non-obvious for someone who did not look into the implementation.

Alternatively, you could run this at the end of this function, it should not be confusing to anyone that certain properties get computed *after* the expression was fully deserialized. 


================
Comment at: clang/test/CXX/class/class.compare/class.spaceship/p1.cpp:106
       Cmp<G2>() <=> Cmp<G2>(), // expected-note-re {{in defaulted three-way comparison operator for '{{.*}}Cmp<{{.*}}G2>' first required here}}j
-      // expected-error@#cmp {{no matching conversion for static_cast from 'void' to 'std::strong_ordering'}}
+      // expected-error@#cmp {{static_cast from 'void' to 'std::strong_ordering' is not allowed}}
       Cmp<H>() <=> Cmp<H>(), // expected-note-re {{in defaulted three-way comparison operator for '{{.*}}Cmp<{{.*}}H>' first required here}}j
----------------
Do you have any idea why did this diagnostic change?


================
Comment at: clang/test/CXX/drs/dr2xx.cpp:161
+    void test6(A::S as) { struct f {}; (void) f(as); } // pre20-error {{no matching conversion}} pre20-note +{{}}
+    // post20-error at -1 {{functional-style cast from 'A::S' to 'f' is not allowed}}
   };
----------------
This might be related to other diagnostic change.
Why do we see the new behavior here? Do you think we should keep the old behavior maybe? 
I feel that both errors are communicating the same thing and probably keeping the current state is acceptable, just want to better understand how we got there.

It definitely has something to do with the initialization code, but I struggle to understand what exactly changed


================
Comment at: clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp:131
+// pre20-error at -2 {{no matching constructor for initialization of 'B'}}
+// post20-error at -3 {{excess elements in struct initializer}}
+// post20-error at -4 {{excess elements in struct initializer}}
----------------
Given how pervasive this error is right now, I feel that we want to add a name of the struct to this message.
This case is also a good example of how this diagnostic can be really low quality with templates: it's unclear which exact base class causes a problem here from the compiler output,.

Maybe open a GH issue for that? It seems like an independent task that will also affect braced initializers and may need test file updates.


================
Comment at: clang/test/PCH/cxx_paren_init.cpp:30
+// CHECK: ret void
\ No newline at end of file

----------------
NIT: add a newline


================
Comment at: clang/test/PCH/cxx_paren_init.h:6
+void bar(int i, int j) { int arr[4](i, j); };
\ No newline at end of file

----------------
NIT: add a newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531