Page MenuHomePhabricator

[clang] Implement sugared substitution changes to infrastructure
ClosedPublic

Authored by mizvekov on Sep 25 2022, 8:24 AM.

Details

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 <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Sep 25 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 8:24 AM
mizvekov requested review of this revision.Sep 25 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 8:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 462735.Sep 25 2022, 8:38 AM
mizvekov edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 8:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mizvekov updated this revision to Diff 462738.Sep 25 2022, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 9:37 AM

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 added a comment.EditedSep 26 2022, 10:17 AM

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!

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++ :-)!

mizvekov removed a reviewer: Restricted Project.Oct 9 2022, 3:51 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 9 2022, 3:51 AM
mizvekov updated this revision to Diff 466365.Oct 9 2022, 5:44 AM
mizvekov removed a reviewer: Restricted Project.Oct 9 2022, 5:45 AM
mizvekov updated this revision to Diff 467920.Oct 14 2022, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 2:31 PM
mizvekov updated this revision to Diff 470008.Oct 23 2022, 1:32 PM
mizvekov retitled this revision from [clang] Instiantiate early substituted entities with sugared template arguments to [clang] Implement sugared substitution changes to infrastructure.
mizvekov edited the summary of this revision. (Show Details)
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
mizvekov updated this revision to Diff 470011.Oct 23 2022, 1:50 PM
mizvekov added reviewers: davrec, Restricted Project, rsmith.Oct 23 2022, 2:57 PM
erichkeane accepted this revision.Oct 24 2022, 6:55 AM
erichkeane added a subscriber: 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.

mizvekov removed a reviewer: Restricted Project.Oct 24 2022, 6:34 PM
This revision is now accepted and ready to land.Oct 24 2022, 6:34 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 24 2022, 6:34 PM
This revision now requires review to proceed.Oct 24 2022, 6:34 PM
mizvekov updated this revision to Diff 470348.Oct 24 2022, 6:34 PM
mizvekov removed a reviewer: Restricted Project.Oct 24 2022, 6:34 PM
This revision is now accepted and ready to land.Oct 24 2022, 6:34 PM
martong removed a subscriber: martong.Oct 25 2022, 12:12 AM
mizvekov updated this revision to Diff 470660.Oct 25 2022, 5:38 PM
mizvekov marked 3 inline comments as done.Oct 25 2022, 5:45 PM