[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.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added inline comments.


================
Comment at: clang/lib/AST/APValue.cpp:1052
+      // FIXME: These should be modeled as having the
+      // LifetimeExtendedTemporaryDecl itself as the base.
+      auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E);
----------------
rjmccall wrote:
> Objective-C object literals are interesting here; arguably, we could allow you to use one as a template argument, and it wouldn't naturally limit linkage.
That makes sense to me; FIXME added.


================
Comment at: clang/lib/AST/ASTContext.cpp:2490
+    ThisAdjustment += getASTRecordLayout(Derived).getBaseClassOffset(Base);
+    RD = Path[I];
+  }
----------------
rnk wrote:
> 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.
While we do support such things in general, we do not support constant-evaluation of a conversion between pointer-to-derived and pointer-to-virtual-base, and there is no `APValue` representation for such a thing. Because we take an `APValue` as input, this function should work correctly even under the MS ABI.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5136
+  case APValue::FixedPoint:
+    llvm_unreachable("Fixed point types are disabled for c++");
+    return;
----------------
rjmccall wrote:
> Can we extract a function to do this mangling that's called from both here and FixedPointLiteral, and then have that assert, rather than leaving a bomb for a corner case of a corner case?
Done. They're somewhat different cases because the other case is actually reachable (via `__attribute__((overloadable))`) but I think it makes sense to assume that we'll eventually support fixed-point in C++ mode. (I think it's a bit strange we didn't support it from the start.)


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5181
+      mangleType(T);
+      Out << "0E";
+      return;
----------------
rjmccall wrote:
> This could also be extracted and shared with the template-argument mangler.  In fact, isn't most of this case redundant with the template-argument mangler?
Theoretically yes, but while there's a lot of overlap between `TemplateArgument`s  and `APValue`s, they're different representations and can represent a somewhat different set of values.

I think we could convert all non-type `TemplateArgument`s to `APValue`s before mangling them, and that might reduce duplication a little, if you'd like?


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1746
+
+  case APValue::AddrLabelDiff:
+  case APValue::FixedPoint:
----------------
rnk wrote:
> Why is this even an APValue kind... O_o
I think the idea is that we want to be able to include these in function-local static dispatch arrays with constant initialization. But yeah, this is by far the weirdest kind of `APValue`.


================
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
----------------
rnk wrote:
> These FIXMEs represent a lot of future work. :(
> 
> Thanks for adding test coverage on both sides, though.
I've asked Jon Caves about this, maybe we'll just be told what the rule is =)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89998