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

Fri Aug 26 04:07:04 PDT 2022

mizvekov added a comment.

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

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

Thanks, and sorry for the long time to respond to this.

I have been thinking about this problem, and I have an idea for a solution which is better on the performance side, but more complex change.

We would need to implement some pattern matching during AST traversal, and keep track of the pack index there. When extracting a component of the larger type, such as when deducing non-pack arguments from a pack, we would wrap it over with a new sugar type node which encodes the current pack indexes.

I reflected a bit on this, and yeah I think we should try to design something which does not potentially hinder large packs so much.

I am not sure how this is going to play out in practice, and it's possible we may need to come back to this solution again, but I think it's prudent to revert it for now if we are not sure, as this is an API change, however obscure for most users.

As an aside, it would be really helpful if you could track how the compilation of your codebase performs as clang evolves, regardless if we keep this change or not :)
Maybe you can reuse something from http://llvm-compile-time-tracker.com/

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113