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

Alexander Kornienko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 24 18:14:27 PDT 2022


alexfh added a comment.

In D128113#3746674 <https://reviews.llvm.org/D128113#3746674>, @mizvekov wrote:

> 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.

For now we've added a workaround for the most problematic use case and got us unblocked. Thus there is no need now in introducing additional flags. However, I'm rather concerned about the cumulative additional overhead our build infrastructure gets as a result of this patch. Measuring this would unfortunately be a project of its own, which I'm not ready to commit to. If reverting this and discussing with Richard Smith is an option for you, I'd suggest doing this now and if you end up not finding a good alternative, we would probably need to find a way to estimate the impact of this patch. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113



More information about the cfe-commits mailing list