<div dir="ltr">On Tue, Mar 26, 2013 at 3: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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Mar 26, 2013, at 2:50 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>
> - Update the test case to pass under x86_64.<br>
> - Implement John's comments.<br>
> - commit attr tablegen changes, add implicit model attr.<br>
><br>
> Hi rjmccall,<br>
<br>
</div>+ std::pair<uint64_t, unsigned> MemPtrInfo =<br>
+ ABI->getMemberPointerWidthAndAlign(MPT);<br>
+ Width = MemPtrInfo.first;<br>
+ Align = MemPtrInfo.second;<br>
<br>
This can just be<br>
llvm::tie(Width,Align) = ABI->getMemberPointerWidthAndAlign(MPT);<br></blockquote><div> </div><div>Nice.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Index: lib/AST/CXXABI.h<br>
===================================================================<br>
--- lib/AST/CXXABI.h<br>
+++ lib/AST/CXXABI.h<br>
@@ -15,6 +15,7 @@<br>
#ifndef LLVM_CLANG_AST_CXXABI_H<br>
#define LLVM_CLANG_AST_CXXABI_H<br>
<br>
+#include "clang/AST/CharUnits.h"<br>
<br>
You don't appear to use anything from this.<br></blockquote><div><br></div><div>Gone.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ unsigned getMemberPointerAlign(const MemberPointerType *MPT) const {<br>
+ const TargetInfo &Target = Context.getTargetInfo();<br>
+ return Target.getTypeAlign(Target.getPtrDiffType(0));<br>
}<br>
<br>
This is now dead.<br></blockquote><div><br></div><div>Gone.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Attr *IA = RD->getAttr<MSInheritanceAttr>();<br>
+ assert(IA && "should have added implicit attribute in sema");<br>
+ attr::Kind Inheritance = IA->getKind();<br>
+<br>
<br>
I'm not thrilled by the idea of forcing the existence of the attribute.<br>
Adding one makes sense as a way to capture the MSVC behavior<br>
of locking in unspecified sizes as soon as a member pointer is asked<br>
for, but maybe<br></blockquote><div><br></div><div>Yeah, I removed those attributes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ // FIXME: Verify that our alignment matches MSVC.<br>
+ unsigned Align = Target.getTypeAlign(Target.getPtrDiffType(0));<br>
+ return std::make_pair(Width, Align);<br>
<br>
Okay, I'm willing to give very good odds that the alignment is the alignment<br>
of the notional member-pointer struct, which works out to alignof(int) if the<br>
member pointer is nothing but ints and alignof(ptrdiff_t) otherwise. You<br>
should compute this in your switch cases.<br></blockquote><div><br></div><div>Done. It looks like function pointers always have a pointer, and data pointers never do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I would also suggest breaking these down first by IsFunc and then by<br>
inheritance model.<br></blockquote><div><br></div><div>Yep.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, it would make sense to have comments that actually describe these<br>
structures. For example, I'm guessing that it's something like this, although<br>
I probably have the fields mixed up:</blockquote><div><br></div><div>[snip]</div><div><br></div><div>That seems pretty close to me, but I hesitate to clean it up and commit it without being certain. :) When I do codegen later I can commit some documentation of the fields with greater confidence.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
+ // 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>
<br>
</div>You go on to describe this fallback mechanism, so I wouldn't call it<br>
poorly-understood any more.<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // FIXME: MSVC picks a model on the first use that needs to know the size,<br>
+ // rather than on the first mention of the type, e.g. typedefs.<br>
<br>
Well, I guess we could hook this into RequireCompleteType.<br></blockquote><div><br></div><div>Hrm? But then the member pointer size might be picked at some earlier point in time where we don't have a complete record definition. I think it's best to wait until we're really going to use a member pointer type.</div>
</div></div></div>