<div dir="ltr"><div>2013/1/16 John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank" class="cremed">rjmccall@apple.com</a>></span><br></div><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 Dec 29, 2012, at 6:48 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank" class="cremed">timurrrr@google.com</a>> wrote:<br>





> Please review the new patch attached.<br>
><br>
> I've put an extra FIXME for the RTTI handling<br>
> and added a new check to the constructor CodeGen test.<br>
<br>
</div>Sorry about the delay.<br></blockquote><div>Please tell me if there's something actionable for me to improve my emails/patches to reduce delays/number of iterations.</div><div>e.g. read these and these docs, read XYZ more carefully, do ABC rather than KLM, etc.</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">
Instead of repeating<br>
  Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft<br>
all over the place, please introduce an isItaniumABI() predicate.<br>
Also, consider caching it as a field of VTableContext.<br></blockquote><div>Unfortunately, I'm afraid havin isItaniumABI() is not very helpful as we may also see CXXABI_ARM.</div><div>Am I right?</div><div>
Instead, I've added the IsMicrosoftABI field to VTableContext together with the isMicrosoftABI() method.<br></div><div><br></div><div>By the way, thanks to the isMicrosoftABI() method it should be easier to find all the places in VTableBuilder.cpp where we have ABI-specific code when we decide to inject some interfaces and abstraction.</div>




<div><br></div><div>Did I get your idea right?</div><div>Or did you mean to add this predicate to ASTContext or TargetInfo ?</div><div><br></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">





+    assert(!isMicrosoft &&<br>
+           "Implicit virtual dtors are not yet supported in MS ABI");<br>
+<br>
<br>
This is not an appropriate use of assert.<br></blockquote><div>I've switched back to llvm_unreachable.</div><div>Please tell me if I'm doing it wrong too!</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">





Otherwise this looks fine.</blockquote><div>Attached is an updated version of the patch, can you please review it?</div><div><br></div><div>Thanks!</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">


<span><font color="#888888"><br>
John.<br>
</font></span></blockquote></div><br></div></div>