OK, will do that. When I have the patch ready - should I wait for another review or can go ahead and commit? (if the latter - I'm willing to fix post-commit issues, if any) Timur Iskhodzhanov, Google Russia
2013/1/18 John McCall <[email protected]>: > On Jan 17, 2013, at 8:15 AM, Timur Iskhodzhanov <[email protected]> wrote: >> 2013/1/17 John McCall <[email protected]>: >>> On Jan 16, 2013, at 6:11 AM, Timur Iskhodzhanov <[email protected]> wrote: >>> >>> 2013/1/16 John McCall <[email protected]> >>>> >>>> On Dec 29, 2012, at 6:48 AM, Timur Iskhodzhanov <[email protected]> >>>> 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. > > - uint64_t AddressPoint = AddressPoints.lookup(Base); > - assert(AddressPoint && "Address point must not be zero!"); > - > - return AddressPoint; > + return AddressPoints.lookup(Base); > > Since you have the isMicrosoftABI() bit now, could you just tweak the > assert to (AddressPoint != 0 || isMicrosoftABI())? > > + void Error(SourceLocation loc, StringRef error); > > I suggest calling this ErrorUnsupported and making the 'error'\ > text a brief description of what's not supported. So you'd call this like: > ErrorUnsupported(loc, "implicit virtual destructors in the Microsoft ABI") > and the diagnostic string you register would be something like: > "v-table layout for %0 is unsupported" > > This makes these diagnostics more consistent (not that that's *so* > important, but still) and also makes it obvious that this is just intended > as a way to report an incomplete implementation and is not intended > as a general error-reporting mechanism. > > Otherwise this looks good. > > John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
