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

[John McCall]

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.

> 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.

> +    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).

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/9a136328/attachment.html>