[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a subscriber: rjmccall.
mizvekov added a comment.

In D126172#3626809 <https://reviews.llvm.org/D126172#3626809>, @roberteg16 wrote:

> Hi again @mizvekov, I've been spending a bit of time on resuming this issue.
>
> When applying (after reverting the changes) the first change related to the `Template` case of profiling the as-written one name this resulted in the following test failing:
> ....
> I've been looking around trying to figure out what's the issue but I am feeling that I lack the required knowledge to diagnose properly the problem.

I think we may be failing to canonicalize template arguments somewhere else where it's required.

We could be for example producing two different types because we produced two different template specializations for the same canonical template arguments.

The most likely suspect is `Sema::CheckTemplateArgumentList` in `SemaTemplate.cpp`.
That function is supposed to receive the arguments as written as input, and then produce a 'resolved' template argument list in the 'Converted' output parameter.

Looking at the helper function `CheckTemplateArgument`, right at the bottom for the Template and TemplateExpansion cases, we are pushing the argument into the Converted list without canonicalizing it.
I think that is one problem.

Back at `CheckTemplateArgumentList`, there are also some cases where we are perfoming pack expansions where we are potentially pushing non-canonical template arguments into that list, this is also worth looking over.

> One question I want to ask is: On `StmtProfiler::VisitTemplateArgument` it states on a comment:
>
>> // Mostly repetitive with TemplateArgument::Profile!
>
> but as far as I could see it really does not mimic the behavior in some cases. Is this at purpose?
>
> Thanks for your time.

That one looks mostly correct, except perhaps that we should be profiling the number of known expansions for the TemplateExpansion case, but that goes for both implementations.

I don't know if this was done on purpose but then became invalid later with other changes, the commits which introduces these things are pretty ancient, they date back to SVN era, and there is a lack of explanations in the history.

We can try pinging whoever was involved at the time. I can't find the phab handle of the author of that commit, but it seems @rjmccall might have been involved and he is still around.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172