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