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

[John McCall via Phabricator via cfe-commits]

rjmccall 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>
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146386