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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
