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