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
