<div dir="ltr"><div style>New patch forthcoming, crossing fingers that it goes to the same thread.</div><div><br></div>On Wed, Mar 20, 2013 at 10:57 PM, 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>On Mar 20, 2013, at 3:59 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>


> CHANGE SINCE LAST DIFF<br>
>  <a href="http://llvm-reviews.chandlerc.com/D558?vs=1333&id=1338#toc" target="_blank">http://llvm-reviews.chandlerc.com/D558?vs=1333&id=1338#toc</a><br>
<br>
<br>
</div>+  // It's a little silly for us to cache this.<br>
+  llvm::IntegerType *getPtrDiffTy() {<br>
<br>
It's actually CGM.PtrDiffTy, so there was never a good reason to cache it<br>
separately.<br></blockquote><div><br></div><div>Nice, nuking this method makes the code shorter.</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">


+llvm::Constant *<br>
+MicrosoftCXXABI::getNullMemberDataPointer(const MemberPointerType *MPT) {<br>
+  assert(MPT->isMemberDataPointer());<br>
+  llvm::Constant *Zero = llvm::ConstantInt::get(getPtrDiffTy(), 0);<br>
+  llvm::Constant *AllOnes = llvm::Constant::getAllOnesValue(getPtrDiffTy());<br>
+  const CXXRecordDecl *RD =<br>
+    cast<CXXRecordDecl>(MPT->getClass()->getAs<RecordType>()->getDecl());<br>
<br>
MPT->getClass()->getAsCXXRecordDecl()<br></blockquote><div><br></div><div>Nice.</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">

+  if (RD->getNumVBases() == 0) {<br>
<br>
I don't think you can safely call this on a forward-declaration.<br></blockquote><div><br></div><div>True.  Based on what I've learned since I wrote this patch, I'll need to go look at those inheritance attributes.</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">
Why don't you make helper functions to create the all-ones and all-zero<br>
ptrdiff_t values?  That would make this code much more succinct.<br></blockquote><div><br></div><div>Done.</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">


+  // FIXME: Emit a pair of {vbtable offset, field offset} when we know vbtable<br>
+  // layout.<br>
+  return GetBogusMemberPointer(QualType(MPT, 0));<br>
<br>
I don't think this ever actually comes up here.  If it did, you'd need a<br>
totally different interface anyway.<br></blockquote><div><br></div><div>Yeah, we do need a different interface.  Testing this case hits assertions.</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">


I believe vb-table offsets are only introduced when you convert across a<br>
virtual base boundary.<br></blockquote><div><br></div><div style>I haven't looked too deeply into it yet, just enough to know how it differs and that we don't support it yet.</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">
+  // For member data pointers, this is just a check against -1 or 0.<br>
+  if (MPT->isMemberDataPointer()) {<br>
+    assert(MemPtr->getType() == getPtrDiffTy());<br>
+    llvm::Constant *Val = getNullMemberDataPointer(MPT);<br>
+    return Builder.CreateICmpNE(MemPtr, Val, "memptr.tobool");<br>
+  }<br>
<br>
You forgot the virtual inheritance case here.<br></blockquote><div><br></div><div style>OK, fixed, that will issue an error instead of asserting now.</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">

+  if (MemPtr->getType() != getPtrDiffTy()) {<br>
+    // If it's not a ptrdiff_t, then it's an aggregate.<br>
+    ErrorUnsupportedABI(CGF, "non-scalar member pointers");<br>
+    return llvm::Constant::getNullValue(PType);<br>
+  }<br>
+<br>
<br>
Shouldn't this be pretty straightforward to implement?  We may not know<br>
how to lay out a vbtbl yet, so the conversion case will still elude us, but we<br>
do know the offset of the vb-table and how to do an offsetted load out of it.<br></blockquote><div> </div><div>I haven't figured out exactly how to do this with a virtual base yet, and Timur says that Clang currently has very little support for virtual inheritance.  I'd rather handle simple member data pointers that are just offsets first, and then come back when we tackle virtual inheritance.<br>
</div></div></div></div>