erichkeane added a comment.
just a pair of minor changes I'd like to see, otherwise this LGTM.
================
Comment at: clang/lib/Sema/SemaTemplateVariadic.cpp:103-106
+ VisitSubstNonTypeTemplateParmPackExpr(SubstNonTypeTemplateParmPackExpr *E)
{
+ Unexpanded.push_back({E, E->getParameterPackLocation()});
+ return true;
+ }
----------------
mizvekov wrote:
> aaron.ballman wrote:
> > Do we need to handle `FunctionParmPackExpr` as well?
> Right, that and SubstTemplateTemplateParmPack are the two missing cases we
> could handle here, and perhaps that would allow us to get rid of that 'from
> outer parameter packs' diagnostics completely.
>
> Would you prefer to handle everything in one patch?
I think I'm alright doing those in a followup patch.
================
Comment at: clang/lib/Sema/SemaTemplateVariadic.cpp:723
+ const auto *ND = ParmPack.first.get<const NamedDecl *>();
+ if (isa<VarDecl>(ND)) {
+ // Figure out whether we're instantiating to an argument pack or not.
----------------
Can you please make this an `else if` at the level higher? The 'else' after
this ends up being so small that it gets lost trying to figure out what is
going on here.
So:
`} else if (const auto *VD = dyn_cast<VarDecl>(ParmPack.first.get<const
NamedDecl *>()))`
================
Comment at: clang/lib/Sema/SemaTemplateVariadic.cpp:865
+ const auto *ND = Unexpanded[I].first.get<const NamedDecl *>();
if (isa<VarDecl>(ND)) {
// Function parameter pack or init-capture pack.
----------------
same comment here, see if the 'vardecl' logic can be moved up a level in the
'if' tree.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128095/new/
https://reviews.llvm.org/D128095
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
