[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

Tue Jun 6 11:40:43 PDT 2023

bolshakov-a added inline comments.

================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4397
+    // argument.
+    // As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111.
+    auto *SNTTPE = cast<SubstNonTypeTemplateParmExpr>(E);
----------------
bolshakov-a wrote:
> efriedma wrote:
> > bolshakov-a wrote:
> > > erichkeane wrote:
> > > > erichkeane wrote:
> > > > > aaron.ballman wrote:
> > > > > > We should get this nailed down. It was proposed in Nov 2020 and the issue is still open. CC @rjmccall 
> > > > > This definitely needs to happen.  @rjmccall or @eli.friedman ^^ Any idea what the actual mangling should be?
> > > > This is still an open, and we need @rjmccall @eli.friedman or @asl to help out here.
> > > Ping @efriedma, @rjmccall, @asl.
> > I'm not really familiar with the mangling implications for this particular construct, nor am I actively involved with the Itanium ABI specification, so I'm not sure how I can help you directly.
> > 
> > That said, as a general opinion, I don't think it's worth waiting for updates to the Itanuim ABI  document to be merged; such updates are happening slowly at the moment, and having a consistent mangling is clearly an improvement even if it's not specified.  My suggested plan of action:
> > 
> > - Make sure you're satisfied the proposed mangling doesn't have any holes you're concerned about (i.e. it produces a unique mangling for all the relevant cases).  If you're not sure, I can try to spend some time understanding this, but it doesn't sound like you have any concerns about this.
> > - Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang.  Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> > - Go ahead with this.
> > Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> 
> I'm sorry for noting one more time that Richard already pushed these changes in clang upstream, but they had been just reverted.
> 
> Maybe, I should make a PR into Itanium API repository, but I probably need some time to dig into the theory and all the discussions. But yes, even NTTP argument mangling rules are not still merged: https://github.com/itanium-cxx-abi/cxx-abi/pull/140
@aaron.ballman, @erichkeane, seems like it is already fixed in the ABI document:
> Typically, only references to function template parameters occurring within the dependent signature of the template are mangled this way. In other contexts, template instantiation replaces references to template parameters with the actual template arguments, and mangling should mangle such references exactly as if they were that template argument.

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.template-param

See also [the discussion in the issue](https://github.com/itanium-cxx-abi/cxx-abi/issues/111#issuecomment-1567486892).

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

https://reviews.llvm.org/D140996