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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked 2 inline comments as done.
mizvekov added inline comments.


================
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.
----------------
erichkeane wrote:
> 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 :)).
> 
> 
Thanks, true about the maximum size :)

Some of the cases could be unified as we have two types and two expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128095