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

[Timur Iskhodzhanov]

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.


> 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?
Instead, I've added the IsMicrosoftABI field to VTableContext together with
the isMicrosoftABI() method.

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.

Did I get your idea right?
Or did you mean to add this predicate to ASTContext or TargetInfo ?

+    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!


> Otherwise this looks fine.

Attached is an updated version of the patch, can you please review it?

Thanks!


>
> John.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/b93efcf3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13231_5.patch
Type: application/octet-stream
Size: 13118 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/b93efcf3/attachment.obj>