This is an archive of the discontinued LLVM Phabricator instance.

[clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type
ClosedPublic

Authored by mizvekov on Aug 28 2022, 1:29 PM.

Details

Summary

This change allows us to represent in the AST some specific
circumstances where we substitute a template parameter type
which is part of the underlying type of a previous substitution.

This presently happens in some circumstances dealing with
substitution of defaulted parameters of template template
parameters, and in some other cases during concepts substitution.

The main motivation for this change is for the future use in the
implementation of template specialization resugaring, as this will
allow us to represent a substitution with sugared types.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Aug 28 2022, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 1:29 PM
mizvekov requested review of this revision.Aug 28 2022, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2022, 1:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 456216.Aug 28 2022, 3:41 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 456217.Aug 28 2022, 3:49 PM
mizvekov retitled this revision from [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type. to [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type.
mizvekov updated this revision to Diff 458623.Sep 7 2022, 7:16 PM
mizvekov updated this revision to Diff 459367.Sep 11 2022, 7:46 AM
mizvekov updated this revision to Diff 460127.Sep 14 2022, 9:00 AM
mizvekov updated this revision to Diff 460592.Sep 15 2022, 6:10 PM
martong removed a subscriber: martong.Sep 16 2022, 12:18 AM
mizvekov updated this revision to Diff 460909.Sep 16 2022, 2:34 PM
mizvekov updated this revision to Diff 461016.Sep 17 2022, 10:43 AM
mizvekov edited the summary of this revision. (Show Details)
mizvekov added reviewers: rsmith, davrec, Restricted Project.Sep 17 2022, 3:55 PM

Patch generally seems OK to me. I would vastly prefer a better commit message explaining the intent here. Also, not quite sure I see the need for the extra bit?

Patch generally seems OK to me. I would vastly prefer a better commit message explaining the intent here. Also, not quite sure I see the need for the extra bit?

Well mechanically the low level intent is explained in the commit summary, and it does give some benefits without any further patches as now we can represent better in the AST some substitutions related to default arguments in template template parameters, and some other substitutions performed in concepts checking.

But the important reason is that with resugaring, this will allow us to produce a resugared type which still contains SubstNodes marking the original substitutions, otherwise we would have to wipe that out and we wouldn't be able to resugar these types again if needed.

The extra bit is for saving storage when the underlying type is already canonical, so that this patch has minimal memory usage impact.

Patch generally seems OK to me. I would vastly prefer a better commit message explaining the intent here. Also, not quite sure I see the need for the extra bit?

Well mechanically the low level intent is explained in the commit summary, and it does give some benefits without any further patches as now we can represent better in the AST some substitutions related to default arguments in template template parameters, and some other substitutions performed in concepts checking.

But the important reason is that with resugaring, this will allow us to produce a resugared type which still contains SubstNodes marking the original substitutions, otherwise we would have to wipe that out and we wouldn't be able to resugar these types again if needed.

The extra bit is for saving storage when the underlying type is already canonical, so that this patch has minimal memory usage impact.

Woops, sorry about the 'extra bit' part, that was because of a comment I had and deleted, I was suggesting at one point that the bit should be a property of the 'underlying type' (that is, determine it based on the canonicalness of the underlying type), but I think I figured out why it wouldn't be necessary.

As far as the "commit summary", the patch summary at least is empty...

As far as the "commit summary", the patch summary at least is empty...

Yeah I meant the one line title of the commit, but I will update it to include a summary with further elaboration in a bit.

davrec accepted this revision.Sep 20 2022, 6:19 AM

Agree this needs a brief commit message, just explaining that previously the underlying type had to be canonical, but now it can be sugared, allowing a better/more informative representation.

clang/include/clang/AST/TypeProperties.td
745–746

^ Delete this comment

mizvekov updated this revision to Diff 461851.Sep 21 2022, 4:46 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 21 2022, 5:23 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.