<div dir="ltr">On Tue, Mar 26, 2013 at 11:08 AM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Mar 22, 2013, at 4:50 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>

>    - Implement John's comments.<br>
<br>
</div>-    std::pair<uint64_t, unsigned> PtrDiffInfo =<br>
-      getTypeInfo(getPointerDiffType());<br>
-    Width = PtrDiffInfo.first * ABI->getMemberPointerSize(MPT);<br>
-    Align = PtrDiffInfo.second;<br>
+    Width = ABI->getMemberPointerWidth(MPT);<br>
+    Align = ABI->getMemberPointerAlign(MPT);<br>
<br>
Please have one function that returns both of these.  No reason to<br>
do two virtual calls when we can do one.<br>
<br>
This will also allow you to get the alignment right more easily on 64-bit.<br></blockquote><div><br></div><div style>Done.  It just didn't seem like the style of the rest of the ASTContext queries.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

+static bool usesMultipleInheritanceModel(const MemberPointerType *MPT,<br>
+                                         const CXXRecordDecl *RD) {<br>
+  // For data pointers, the representation is the same for single and multiple<br>
+  // inheritance classes.  We return false to use the single inheritance model.<br>
+  if (MPT->isMemberDataPointerType())<br>
+    return false;<br>
+<br>
<br>
This isn't quite what I meant.  The top-level routine already decides whether<br>
the member type is a function or not.  Just don't call this function if it is.<br></blockquote><div><br></div><div style>I'm adding the implicit attribute now, so I don't think I can do this optimization any more.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
That is, change this:<br>
+    else if (usesMultipleInheritanceModel(MPT, RD))<br>
to<br>
 +    else if (isFunc && usesMultipleInheritanceModel(MPT, RD))<br>
<br>
+  if (RD->getTypeForDecl()->isIncompleteType()) {<br>
<div class="im">+    if (Attr *IA = RD->getAttr<SingleInheritanceAttr>())<br>
+      Inheritance = IA->getKind();<br>
+    else if (Attr *IA = RD->getAttr<MultipleInheritanceAttr>())<br>
+      Inheritance = IA->getKind();<br>
+    else if (Attr *IA = RD->getAttr<VirtualInheritanceAttr>())<br>
+      Inheritance = IA->getKind();<br>
</div>+    else<br>
+      Inheritance = attr::UnspecifiedInheritance;<br>
+  } else {<br>
<br>
Check for the attribute first, and check for it with:<br>
  if (Attr *IA = RD->getAttr<MSInheritanceAttr>()) {<br>
  }<br></blockquote><div><br></div><div style>Much better.  :)  There needs to be a diagnostic if we have multiple inheritance attrs, though.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

That way you can e.g. implicitly add your UnspecifiedInheritanceAttr to a<br>
class decl and it'll just work.  Also it means you don't have to enumerate your<br>
inheritance attribute kinds.<br>
<br>
+  // If template instantiation fails or the type is just incomplete, the ABI has<br>
+  // a poorly understood fallback mechanism in place, so we continue without any<br>
+  // diagnostics.<br>
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft())<br>
+    RequireCompleteType(Loc, Class, 0);<br>
<br>
This would be an excellent opportunity to implicitly add your<br>
UnspecifiedInheritanceAttr to all the existing declarations. :)<br></blockquote><div><br></div><div style>Is it OK if I just implicitly add the right attribute if there isn't one present already?  I suppose the unspecified model is the only one that I need to add for correctness.</div>
<div style><br></div><div style>It's actually important to lock this in at some point so we don't have different sizes at different TU points.  In MSVC, if you declare a member pointer variable before the class definition, it uses the unspecified model.  If you wait until after the definition, it will pick a more restrictive model.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The changes to ClangAttrEmitter look fine.</blockquote><div><br></div><div style>Committed separately as r178054.</div></div></div></div>