[PATCH] D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:2490
+    ThisAdjustment += getASTRecordLayout(Derived).getBaseClassOffset(Base);
+    RD = Path[I];
+  }
----------------
rjmccall wrote:
> What should we do on targets that allow virtual bases in member pointer conversions?
It looks like the MS ABI codepaths don't use this routine. If we could put it somewhere Itanium-specific that would be accessible to the mangler and CGCXXABI implementation, that would be ideal, but ASTContext doesn't seem too bad.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:657-660
   // <member-function-pointer> ::= $1? <name>
   //                           ::= $H? <name> <number>
   //                           ::= $I? <name> <number> <number>
   //                           ::= $J? <name> <number> <number> <number>
----------------
rsmith wrote:
> For what it's worth, I'm fairly convinced the `$` is not actually part of the mangling of the member function pointer, and is instead part of the mangling of a non-type template argument (or more generally of a value).
That makes sense to me. I'll see if I can simplify in a follow-up.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:751-752
+  // to convert every integer to signed 64 bit before mangling (including
+  // unsigned 64 bit values). Do the same, but preserve bits beyond the bottom
+  // 64.
+  llvm::APInt Value =
----------------
IIUC, this means we can mangle __int128 non-type template parms now? Let's add a test for that.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1746
+
+  case APValue::AddrLabelDiff:
+  case APValue::FixedPoint:
----------------
Why is this even an APValue kind... O_o


================
Comment at: clang/test/CodeGenCXX/mangle-class-nttp.cpp:27
+#ifndef _WIN32
+// FIXME: MSVC crashes on the first of these and mangles the second the same as
+// the nullptr version. Check the output is correct once we have a reference to
----------------
These FIXMEs represent a lot of future work. :(

Thanks for adding test coverage on both sides, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89998