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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D128113#3745888 <https://reviews.llvm.org/D128113#3745888>, @alexfh wrote:

> The main questions we need to answer here are:
>
> 1. is the cost of storing parameter pack substitution indices while building AST worth the benefits?
> 2. is there an alternative solution that would shift the cost to the moment when this information is about to be used (when formatting diagnostic, I suppose)?
> 3. is there a good way to make this cost optional (using a flag or somehow else)?



1. For me yes, this was a fairly simple patch that got reviewed quickly and got the job done. But it seems it was not good for your use case :)
2. I think there is a chance the approach I mentioned earlier could work. It's hard for me to tell without spending some time trying. This could though end up being a much more complicated patch, which hits my projects bottleneck: it's pretty hard to find reviewers for this sort of stuff.
3. That has the advantage of change simplicity, but not user simplicity. It could provide a escape hatch in allowing us to have a simple solution now, but then iterate to something better later. But since this is an externally visible change to the AST, it does seem far from ideal to be changing this twice. It would be pretty unfortunate if someone else started using this feature and then we decided to take it away.

I would be fine though reverting this, and then doing some more investigation, and also perhaps waiting for a second opinion from @rsmith.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113