[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

Sat Aug 27 08:05:11 PDT 2022

davrec added a comment.

Two last thoughts:
1: To reiterate it doesn't seem right to view this (or any patch adding not-semantically-relevant info to the AST) as a one-size-fits-all costs vs. benefits optimization.  On the one hand, the additional AST info hurts compilation performance.  On the other, the info might make debugging faster, or be useful to a tool.  A flag governing how much non-semantically-necessary info to add to the AST really seems the better solution in general, and warrants more discussion in cases like this (e.g. Matheus's other resugaring patches, which may cause this debate to resurface regardless of the approach he takes here).  The user could set the flag high when debugging, and decrease it when diagnostics/debug info are less expected/not needed.  (In light of the performance tests done here, the performance benefit of allowing the user to omit *other* non-semantically-necessary nodes from the AST could be significant; such a flag could allow that.)

2: After thinking about it further I don't think the pack index provides sufficiently useful info in any case, since packs will always be expanded in full, in order: when you find the first `SubstTemplateTypeParmType` expanded from a pack, the rest are sure to be right behind.  IIUC I see how including the pack index makes resugaring more straightforward: the substitution of the sugared info for the non-sugared info occurs via a TreeTransform (see clang/lib/Sema/SemaTemplate.cpp in https://reviews.llvm.org/D127695), and by storing the pack index Matheus can simply override `TransformSubstTemplateTypeParmType` to make use of the pack index to easily fetch the corresponding sugared info.  But since the pack indices are predictable given a bird's eye view of the AST, maybe state info can be stored in the TreeTransform to allow the pack index to be inferred in each call to `TransformSubstTemplateTypeParmType`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113