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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
