[cfe-commits] [Review] First step towards PR13231 - vftable generation with -cxx-abi microsoft

[Timur Iskhodzhanov]

2013/1/17 John McCall <rjmccall at apple.com>:
> On Jan 16, 2013, at 6:11 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>
> 2013/1/16 John McCall <rjmccall at apple.com>
>>
>> On Dec 29, 2012, at 6:48 AM, Timur Iskhodzhanov <timurrrr at google.com>
>> wrote:
>> > Please review the new patch attached.
>> >
>> > I've put an extra FIXME for the RTTI handling
>> > and added a new check to the constructor CodeGen test.
>>
>> Sorry about the delay.
>
> Please tell me if there's something actionable for me to improve my
> emails/patches to reduce delays/number of iterations.
> e.g. read these and these docs, read XYZ more carefully, do ABC rather than
> KLM, etc.
>
>
> It's really just that I've been very busy recently, as you can probably tell
> from my commit history. :)  I don't have an immediate solution.

I'm curious if I could write simpler/shorter patches to not exceed
your time slice and get the reviews scheduled more often :)

>> Instead of repeating
>>   Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft
>> all over the place, please introduce an isItaniumABI() predicate.
>> Also, consider caching it as a field of VTableContext.
>
> Unfortunately, I'm afraid havin isItaniumABI() is not very helpful as we may
> also see CXXABI_ARM.
> Am I right?
>
>
> The ARM ABI is a variant of the Itanium ABI;  my intent was that
> isItaniumABI would return true for it.  But it's not that important;
> caching IsMicrosoftABI is fine for now.

OK

>> +    assert(!isMicrosoft &&
>> +           "Implicit virtual dtors are not yet supported in MS ABI");
>> +
>>
>> This is not an appropriate use of assert.
>
> I've switched back to llvm_unreachable.
> Please tell me if I'm doing it wrong too!
>
>
> Neither of these is appropriate!  The compiler should never crash just
> because something is unsupported.  Consider adding a method to emit
> an "unsupported" diagnostic here (you can follow IRGen's lead).

Oh I see now. Actually this was my intent to crash early rather than
continuing (works better in my setup) but I agree with a more general
practice.

Please see a patch with diagnostics attached.

Just in case, I've checked that ImplicitVirtualDtor has the class's
location set up by temporarily replacing
  if (isMicrosoftABI())
with
  if (1)
and ran test/CodeGenCXX/vtable-layout.cpp which has a virtual implicit
destructor test for Itanium ABI.
I can replace with RD->getLocation() if you think this is more appropriate.

Thanks,
Timur
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13231_6.patch
Type: application/octet-stream
Size: 13841 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130117/c2506b17/attachment.obj>