[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

[Andrey Ali Khan Bolshakov via Phabricator via cfe-commits]

bolshakov-a added inline comments.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:720
+    const CXXRecordDecl *RD, const ValueDecl *VD) {
+  MSInheritanceModel IM = RD->getMSInheritanceModel();
+  // <nttp-class-member-data-pointer> ::= <member-data-pointer>
----------------
rjmccall wrote:
> efriedma wrote:
> > It's not obvious to me why the inheritance model is relevant here.  If you want to check if the class has virtual bases, please do that explicitly.  (Note that the inheritance model can be messed with using attributes; see https://learn.microsoft.com/en-us/cpp/cpp/inheritance-keywords.)
> It would at least be good to test what exactly this is based on.
> 
> If RD is the base class of the member pointer type, isn't RD followed by the unqualified name of VD potentially ambiguous when there's any inheritance at all?  You don't even need multiple inheritance, just the presence of multiple classes in the hierarchy that declare a field with the same name.  I mean, if that's what MSVC does then we need to match it, but we should make certain by testing member pointers at different positions in the hierarchy of the declared base.
> 
> (And this is assuming the MSVC model that restricts what subclass member pointers are allowed in superclass member pointer types.)
@efriedma, MS inheritance model overrides real inheritance. I've added a couple of test cases to reflect this. Btw, thanks for the link! I really didn't understand what "unspecified inheritance" is and how it is even possible.

@rjmccall, thank you! I've updated the mangling to a more correct algorithm.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1857
         T->castAs<MemberPointerType>()->getMostRecentCXXRecordDecl();
     const ValueDecl *D = V.getMemberPointerDecl();
     if (T->isMemberDataPointerType())
----------------
efriedma wrote:
> It might be more clear here to use `APValue::isNullPointer()`, instead of checking if `getMemberPointerDecl()` returns null.  At first glance, it's not really obvious why `getMemberPointerDecl()` would return null.  And in theory, it's possible to have a non-null APValue where `getMemberPointerDecl()` returns null (although in this context, such cases should get rejected earlier).
`APValue::isNullPointer()` works only with `LValue`s, not with `MemberPointer`s:
https://github.com/llvm/llvm-project/blob/llvmorg-17-init/clang/lib/AST/APValue.cpp#L1000


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

https://reviews.llvm.org/D146386