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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4579
+      if (CE->hasAPValueResult())
+        mangleValueInTemplateArg(ParamType, CE->getResultAsAPValue(), false,
+                                 /*NeedExactType=*/true);
----------------
I'm not sure what the point of the `if (CE->hasAPValueResult())` is; are you just trying to avoid copying the APValue?  (If this is going to be a repeating pattern, maybe we can add some sort of utility class to represent the pattern.)


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


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

https://reviews.llvm.org/D140996