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

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
     case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+    case VarDecl::ParenListInit:
+      JOS.attribute("init", "paren-list");
----------------
ilya-biryukov wrote:
> NIT: maybe use the same formatting as other switch cases for constistency?
Unfortunately clang-format insists that these be on separate lines.


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast<InitListExpr>(ExprToVisit))
+    filler = ILE->getArrayFiller();
----------------
ilya-biryukov wrote:
> - Should we have a filler for `CXXParenInitListExpr` too?
>   It seems like an important optimization and could have large effect on compile times if we don't 
> 
> - Same question for semantic and syntactic-form (similar to `InitListExpr`): should we have it here?
>   I am not sure if it's semantically required (probably not?), but that's definitely something that `clang-tidy` and other source tools will rely on.
> 
> We should probably share the code there, I suggest moving it to a shared base class and using it where appropriate instead of the derived classes.
> Should we have a filler for CXXParenInitListExpr too? It seems like an important optimization and could have large effect on compile times if we don't

This feels like premature optimization - presumably, wouldn't this only be an issue with extraordinarily large (say, O(1000)) arrays?

> Same question for semantic and syntactic-form (similar to InitListExpr): should we have it here? I am not sure if it's semantically required (probably not?), but that's definitely something that clang-tidy and other source tools will rely on

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic form would be the enclosing `ParenListExpr`.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6169
+          Failure == FK_ConstructorOverloadFailed &&
+          getFailedCandidateSet().size() == 3) {
+        // C++20 [dcl.init] 17.6.2.2:
----------------
ilya-biryukov wrote:
> It seems shaky to rely on the size of `getFailedCandidateSet`.
> What are we trying to achieve here? Is there a more direct way to specify it?
Done.

As mentioned in the above comment, we try to parse this as a `CXXParenListInit` if the only failed overload constructor candidates are the default constructor, the default copy constructor, and the default move constructor).


================
Comment at: clang/lib/Sema/SemaInit.cpp:6188
+        //      (6.7.7), and there is no brace elision.
+        TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
+                                          /*VerifyOnly=*/true);
----------------
ilya-biryukov wrote:
> NIT: maybe move the full comment into the body of the function?
> It describes in detail what the body of the function does and it would be easier to understand whether the code fits the intention in the standard.
> Having the first part of the comment here would still be useful, probably.
Done. Most of this comment is spread out throughout the body of the function anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531