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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/AST/APValue.cpp:1047
+      if (MergeLV(getLVForType(*TI.getType(), computation)))
+        break;
+    } else if (const Expr *E = V.getLValueBase().dyn_cast<const Expr *>()) {
----------------
I'm not sure what ABIs you're thinking about, but the *symbol* linkage/visibility of the type info isn't important, just its formal linkage/visibility.


================
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);
----------------
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.


================
Comment at: clang/lib/AST/ASTContext.cpp:2490
+    ThisAdjustment += getASTRecordLayout(Derived).getBaseClassOffset(Base);
+    RD = Path[I];
+  }
----------------
What should we do on targets that allow virtual bases in member pointer conversions?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4924
+/// Determine whether a given value is equivalent to zero-initialization for
+/// the purpose of discarding a trailing portion of a 'tl' mangling.
+static bool isZeroInitialized(QualType T, const APValue &V) {
----------------
Probably worth reemphasizing the general principle that this is not equivalent to asking whether it has a zero bit-pattern.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5132
+    mangleFloat(V.getFloat());
+    Out << 'E';
+    return;
----------------
Surely we have this as a subroutine already, or could.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5136
+  case APValue::FixedPoint:
+    llvm_unreachable("Fixed point types are disabled for c++");
+    return;
----------------
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?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5155
+      Out << 'E';
+    }
+    Out << 'E';
----------------
These can use the float-mangling subroutine that you should extract.

The condition here needs to be `!isPosZero()`, I think.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5181
+      mangleType(T);
+      Out << "0E";
+      return;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89998