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

Mon Sep 19 07:21:34 PDT 2022

erichkeane added a comment.

In D132816#3799535 <https://reviews.llvm.org/D132816#3799535>, @mizvekov wrote:

> In D132816#3799484 <https://reviews.llvm.org/D132816#3799484>, @erichkeane wrote:
>
>> 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...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132816