[PATCH] D128095: [clang] Improve diagnostics for expansion length mismatch

[Erich Keane via Phabricator via cfe-commits]

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