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

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Happy enough, LGTM.



================
Comment at: clang/lib/Sema/SemaTemplateVariadic.cpp:860
+    } else if (const auto *ND = Unexpanded[I].first.get<const NamedDecl *>();
+               isa<VarDecl>(ND)) {
+      // Function parameter pack or init-capture pack.
----------------
mizvekov wrote:
> mizvekov wrote:
> > erichkeane wrote:
> > > This pattern with the init + condition is a little awkward here... any reason to not just use the cast around the 'ND" and just use the VD in the use below? or is there a good reason to split it up like this?
> > > 
> > > Same with the above case.
> > No very strong reason than that just from my different perception this looked fine :)
> > 
> > Minor advantage that we don't make the variable a VarDecl pointer if we don't need to access it as such.
> > 
> > But no strong preference here, I can have another look tomorrow.
> I played a little bit with this change.
> 
> I think one thing that makes that pattern of adding separate ND and VD a bit confusing is that at a glance it almost looks like these are different cases in the PointerUnion variant. You have to do a double take to see that, since the nested cast is a bit subtle. This can become even subtler as we add more cases in the next patch.
> 
> Or we could stop using PointerUnion on the next patch, since it's so unergonomic with so many variants.
> 
> Anyway, I did some other refactorings and I think in general this will be much clearer to read on my next push.
> 
> With this refactoring, on this part here that problem goes away since we make this section produce a NamedDecl.
> 
> On the second part, below, I tidied up the code so much that I think that nested else isn't almost invisible anymore, since the other blocks become about the same size.
> 
> Otherwise, let me know what you think.
> 
> I also added to this first part here a FIXME comment describing a pre-existing problem where if we get a canonical TTP, the diagnostics fail to point to the relevant parameter since we won't have it's identifier.
> 
> Will try to add a repro and fix for that on the next patch.
Thanks for spending time on this... the nested conditionals on the var-decl was hiding 90%+ of what was going on in the branch, and at least this top one is BETTER enough than before.  I too hate the pointer union (note BTW, that _4_ is the maximum number of elements thanks to 32 bit platforms, that'll burn you :)).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128095