zyn0217 wrote: > @zyn0217 > > > > I am having second thoughts on both this PR and the follow up fix > https://github.com/llvm/llvm-project/commit/adb0d8ddceb143749c519d14b8b31b481071da77 > > > > We shouldn't be adding information that is required for correct substitution > into `SubstTemplateTypeParmType `, it's sugar after all and doesn't survive > canonicalization. That's the reason we have created > `SubstTemplateTypeParmPackType`, that one is canonical and is able to hold > out information so we can finish substitution correctly later. > > > > I think neither of these patches should have touched > `SubstTemplateTypeParmType`, this is probably an issue with not holding out > into `SubstTemplateTypeParmPackType`. > > > > I know I reviewed the follow up patch, but I must have missed it was not > touching the 'Pack' version of the node instead.
Thanks for revisiting the patch. I think it's worth exploring the idea. I'll take a look if I can move that to the Pack node. (Or if you have a patch, let me know so we don't step on each others' work) I added that to the non-Pack node in the first place because of the fact that the node already has an optional Index to track exactly what I want, while the Pack node doesn't. And if I recall it correctly, the Pack node needs an expanded template arguments, and it needs some effort to find out that at the point we rebuild the constraint. So I took the shortcut. https://github.com/llvm/llvm-project/pull/109518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
