[clang] Implement sugared substitution changes to infrastructure

[mizvekov]

Summary

Implements the changes required to perform substitution with
non-canonical template arguments, and to 'finalize' them
by not placing 'Subst' nodes.

A finalized substitution means we won't resugar them later,
because these templates themselves were eagerly substituted
with the intended arguments at the point of use. We may still
resugar other templates used within those, though.

This patch does not actually implement any uses of this
functionality, those will be added in subsequent patches,
so expect no changes to existing tests.

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

[ldionne]

As explained in more details here (https://reviews.llvm.org/D133249#inline-1289205), it would be nice to stop using the DELETE.ME idiom to kick off libc++ CI, unless you really think there's a good chance to cause failures in libc++ specifically.

TLDR of the link: the libc++ CI has 50+ jobs, exactly one of which will exercise your Clang changes. All the other jobs are using pre-compiled versions of Clang. So in reality, this DELETE.ME file trick only results in wasting libc++ CI resources, in addition to adding the libc++ review group on reviews that we probably don't need to look at. If you do suspect that the change might break libc++ specifically (such as changing diagnostics or whatever), then you're welcome to do so, although in the mid-term we'll try to simply run the libc++ tests as part of Clang's own CI setup.

[mizvekov]

In D134604#3814986, @ldionne wrote:

As explained in more details here (https://reviews.llvm.org/D133249#inline-1289205), it would be nice to stop using the DELETE.ME idiom to kick off libc++ CI, unless you really think there's a good chance to cause failures in libc++ specifically.

TLDR of the link: the libc++ CI has 50+ jobs, exactly one of which will exercise your Clang changes. All the other jobs are using pre-compiled versions of Clang. So in reality, this DELETE.ME file trick only results in wasting libc++ CI resources, in addition to adding the libc++ review group on reviews that we probably don't need to look at. If you do suspect that the change might break libc++ specifically (such as changing diagnostics or whatever), then you're welcome to do so, although in the mid-term we'll try to simply run the libc++ tests as part of Clang's own CI setup.

Hello!

I was aware of those discussions, and I even made a workaround to avoid this extra cost of running unrelated pipelines.
The patch on the very bottom of this stack (D134079) disables all the other pipelines except the bootstrapping one.

For this patch, I only needed a pipeline run for the initial submission, but I don't plan to keep running it, I will remove the DELETE.ME on the next submission.

I have two other patches in the stack which I will need to keep running the libcxx-ci as I update them: D131858 and D127695. These have had non-trivial libcxx breakages, and I have even seen new problems introduced as you keep working on the ranges implementation.

Thanks by the way for the awesome work on the libcxx-CI, and I hope in the near future these workarounds will not be needed anymore!

[ldionne]

In D134604#3815634, @mizvekov wrote:

I was aware of those discussions, and I even made a workaround to avoid this extra cost of running unrelated pipelines.
The patch on the very bottom of this stack (D134079) disables all the other pipelines except the bootstrapping one.

Oh, awesome, I wasn't aware of that. Okay, this mitigates a lot of my concern then. The only remaining annoyance is the additional traffic on the libc++ review group, but that's manageable if it gets you the testing you need until we've improved the CI situation w/ Clang.

I have two other patches in the stack which I will need to keep running the libcxx-ci as I update them: D131858 and D127695. These have had non-trivial libcxx breakages, and I have even seen new problems introduced as you keep working on the ranges implementation.

Understood, in that case I think your usage of the CI is definitely justified. I mostly wanted to get the word out that this is not a pattern we want to encourage more generally, for the reasons explained, but I think it doesn't apply here.

Thanks by the way for the awesome work on the libcxx-CI, and I hope in the near future these workarounds will not be needed anymore!

Hey, thanks to you for wanting to ensure your patches don't break libc++ :-)!

[erichkeane]

A couple of nits, otherwise I don't have to see this again.

clang/include/clang/AST/TemplateName.h

158

Based on this comment, should this be 'Finalize' instead of 'Final'? Or something like "ShouldFinalize"?

clang/include/clang/Sema/Template.h

104–105

Hrmph, this additional flag likely requires rebasing/rebuilding on a patch I just committed, which uses this MLTAL again. Hopefully not too bad to propagate.

clang/lib/AST/ASTContext.cpp

3049

typically we name these sorts of things based on their return type, so this would be more hasCanonicalTemplateArguments. BUT the modifying nature of it makes this an awkward function all the same.

I think I'd suggest putting both of the 'output's here on the same side of the function, either both a return, or both an out-param, so the name isn't ambiguous/confusing like this.