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.
----------------
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.
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
