[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.

Sorry for not reviewing this sooner. This looks very good overall, but I still have some NITs and a few major comments. For the major points see:

- the comment about the filler and the syntactic/semantic form for the newly added expression,
- the comment about relying on `getFailedCandidateSet().size()`.



================
Comment at: clang/lib/AST/ExprCXX.cpp:1776
+}
\ No newline at end of file

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


================
Comment at: clang/lib/AST/ExprConstant.cpp:9988
+      }
+    } else
+      llvm_unreachable(
----------------
NIT: maybe add braces as the other branches have them? This seems to be in the [style guide](https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements):
> readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members


================
Comment at: clang/lib/AST/ExprConstant.cpp:10957
+  unsigned NumElts = InitExprs.size();
+  Result = APValue(APValue::UninitArray(), NumElts, NumElts);
+
----------------
Could we add an assertion that the array size and the `InitExprs.size()` are the same?
E.g. it can differ in the case of `InitListExpr` (probably due to the filler optimization?), it would be nice to add a sanity check here.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
     case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+    case VarDecl::ParenListInit:
+      JOS.attribute("init", "paren-list");
----------------
NIT: maybe use the same formatting as other switch cases for constistency?


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:479
 void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
-                                   QualType ArrayQTy, InitListExpr *E) {
-  uint64_t NumInitElements = E->getNumInits();
+                                   QualType ArrayQTy, Expr *ExprToVisit,
+                                   ArrayRef<Expr *> Args) {
----------------
NIT: maybe add a comment that `ExprToVisit` is either `InitListExpr` or `CXXParenInitListExpr`?


================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+  Expr *filler = nullptr;
+  if (auto *ILE = dyn_cast<InitListExpr>(ExprToVisit))
+    filler = ILE->getArrayFiller();
----------------
- 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.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6169
+          Failure == FK_ConstructorOverloadFailed &&
+          getFailedCandidateSet().size() == 3) {
+        // C++20 [dcl.init] 17.6.2.2:
----------------
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?


================
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);
----------------
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.


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2174
+  E->NumExprs = Record.readInt();
+  for (unsigned I = 0; I < E->NumExprs; I++)
+    E->getTrailingObjects<Expr *>()[I] = Record.readSubExpr();
----------------
Could we assign the `CXXParenInitListExpr.NumExprs` on construction and add the assertion sanity-checking the numbers here:
```
unsigned NumExprs = Record.readInt();
(void)NumExprs; // to avoid unused warnings in NDEBUG builds.
assert(NumExprs == E->NumExprs);
```

(Looked up how `DesignatedInitExpr` does a similar thing, probably good for consistency in addition to added safety)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531