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. 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
bug_13231_6.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
