[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values
Alan Zhao via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 18 16:01:51 PST 2022
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
More information about the cfe-commits
mailing list