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