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

[Matheus Izvekov via Phabricator via cfe-commits]

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