[clang] Improve diagnostics for expansion length mismatch

[mizvekov]

Summary

When checking parameter packs for expansion, instead of basing the diagnostic for
length mismatch for outer parameters only on the known number of expansions,
we should also analyze SubstTemplateTypeParmPackType and SubstNonTypeTemplateParmPackExpr
for unexpanded packs, so we can emit a diagnostic pointing to a concrete
outer parameter.

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

.

[mizvekov]

.

[mizvekov]

rebase

[erichkeane]

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

[mizvekov]

In D128095#3712343, @erichkeane wrote:

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

Done.

I split this up into two patches:

• D131802: Introduces the test and a simple fix.
• This one which improves the analyzer to give better diagnostics.

[shafik]

clang/lib/Sema/SemaTemplateDeduction.cpp

741–745

Structured bindings maybe?

[mizvekov]

clang/lib/Sema/SemaTemplateDeduction.cpp

741–745

Sure, they weren't an option back when this patch was first submitted :)

[mizvekov]

ping

[aaron.ballman]

clang/lib/Sema/SemaTemplateVariadic.cpp

103–106

Do we need to handle FunctionParmPackExpr as well?

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

103–106

Right, that and SubstTemplateTemplateParmPack are the two missing cases we could handle here, and perhaps that would allow us to get rid of that 'from outer parameter packs' diagnostics completely.

Would you prefer to handle everything in one patch?

[erichkeane]

just a pair of minor changes I'd like to see, otherwise this LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp

103–106

I think I'm alright doing those in a followup patch.

699

Can you please make this an else if at the level higher? The 'else' after this ends up being so small that it gets lost trying to figure out what is going on here.

So:

} else if (const auto *VD = dyn_cast<VarDecl>(ParmPack.first.get<const NamedDecl *>()))

844

same comment here, see if the 'vardecl' logic can be moved up a level in the 'if' tree.

[aaron.ballman]

clang/lib/Sema/SemaTemplateVariadic.cpp

103–106

I'm also fine handling it in a follow-up if the changes are involved.

[erichkeane]

clang/lib/Sema/SemaTemplateVariadic.cpp

845

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.

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

845

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.

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

845

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.

[erichkeane]

Happy enough, LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp

845

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

[mizvekov]

clang/lib/Sema/SemaTemplateVariadic.cpp

845

Thanks, true about the maximum size :)

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