davrec added a comment.

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

> @davrec  @alexfh
>
> I finally managed to have a talk to @rsmith about this.
>
> He thinks that, even if we can't come up with a better solution in time, the 
> benefits of this patch justify the costs observed, as those substitution 
> nodes are pretty useless without a way to index the pack, and having a rich 
> AST is one of Clang's main goals.

I support that - re-merge for now, and consider ways to reduce costs going 
forward.

I thought this through every which way and could not avoid @mizvekov's 
conclusion, that the only other way would be another Subst* type node, from 
which the pack indices could be inferred, but only via non-straightforward code 
requiring a bird's eye view of the AST.

Taking a step back and reconsidering, it's clear the original way - just 
storing the pack index - is very much preferable to all that complexity.  The 
benefits justify the costs for most users, and for clang developers who would 
be burdened by the appearance of an obscure new type class.  And as I said 
previously I think broad flags governing how much sugar/non-semantic info is 
added to the AST, i.e. letting users balance benefits vs. costs for themselves, 
is the better way to handle these concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to