>> This patch set implements functionality in Casting.h that will >> automatically detect upcasts and optimize away dynamic checks for >> them. > > A little Devil's Advocate: what would happen if we just had upcasting > with Casting.h be a compile time error? It seems like that would > simplify code & make it more legible.
That is another possibility I pondered. If you want to try it out, you can apply just the attached patch `die-on-upcast.patch` and see what build errors you get; I'm seeing quite a bit of stuff, not all of which looks clearly bad. While it would be nice to make it an error since theoretically the programmer should be aware of this, my intuition is that there will be circumstances, such as inside of templates or other code where the cast is not being directly written out by the programmer, that this will impose undue hardship. One of the first errors I saw was inside DEFINE_TRANSPARENT_OPERAND_ACCESSORS(UnaryInstruction, Value), which seems at first glance to fall under that category of "the programmer didn't directly write it". Nonetheless, it might be interesting to turn this on and see what it digs up. Thankfully most of the work I've done here in this patch set is cleanup, so it is orthogonal and would need to happen anyway. The main goal of the patch is mostly to just factor out this kind of upcast (see near the bottom of <http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-October/054135.html>), and retaining the current behavior was the natural thing to do. -- Sean Silva On Sun, Oct 7, 2012 at 6:19 PM, David Blaikie <[email protected]> wrote: > On Sun, Oct 7, 2012 at 2:04 PM, Sean Silva <[email protected]> wrote: >> This patch set implements functionality in Casting.h that will >> automatically detect upcasts and optimize away dynamic checks for >> them. > > A little Devil's Advocate: what would happen if we just had upcasting > with Casting.h be a compile time error? It seems like that would > simplify code & make it more legible. > >> It also simplifies implementing LLVM-style RTTI since now one >> fewer classof() is required. > > Sounds great. > >> The core of the patch set is 0002, which introduces the specialization >> of isa_impl<>. I would appreciate if someone doublechecks my use of >> templates there. >> >> Patches 0001-0005 are for LLVM, 0006-0007 are for Clang. I've been >> running `check` and `check-clang` throughout development, except where >> the refactoring turned up existing bugs (that I fixed an made their >> own patch and put before the refactoring). >> >> Chris, I did manage to find some classof() of the form: >> static bool classof(const Foo *) { return true; } >> where it is not done to optimize the upcast. In particular, there are >> two in Operator.h which basically bless using cast<> to convert >> Instruction's and ConstantExpr's to Operator (this seems to lead to UB >> actually; I started a thread on LLVMdev about it). >> >> I also found quite a bit of cargo-cult'ing around classof. There were >> many things like: >> >> class Base { >> [...] >> static bool classof(const Base *) { return true; } >> static bool classof(const Derived *) { return true; } >> static bool classof(const OtherDerived *) { return true; } >> }; >> >> When the single Base one would suffice. After this patch set none of >> these three are necessary anymore :) >> >> >> Patch descriptions: >> >> 0001-Remove-buggy-classof.patch >> This patch removes a buggy classof that has laid dormant since r55022. >> >> 0002-Casting.h-Automatically-handle-isa-Base-Derived.patch >> This is the core of the patch set. It adds a specialization of >> isa_impl<> that automatically allows upcasts. Includes two unittests >> of the new functionality. The first checks that the upcast is >> automatically inferred, and the second one checks that the mechanism >> does not fall back to an explicit check when the cast is statically >> known to be OK (i.e., that the dynamic check is optimized away). >> >> 0003-docs-Update-HowToSetUpLLVMStyleRTTI.patch >> Documentation update to reflect changes in the previous patch. >> >> 0004-Remove-unnecessary-classof-s.patch >> A mass removal of now-unnecessary classof()'s. >> >> 0005-docs-Improve-HowToSetUpLLVMStyleRTTI.patch >> More documentation improvements regarding LLVM-style RTTI. >> >> 0006-Add-missing-classof.patch: >> Bugfix. Adds a missing classof that manifested itself by triggering a >> build failure when the next patch is applied. This bug is actually >> kind of scary, since what was happening is that the check >> To::classof(&Val) would use the parent class's classof(), yielding a >> completely wrong dynamic check! I'm pretty much positive that there >> are other instances of this bug somewhere in the codebase. I can't >> think of a way to have the compiler enforce that a parent's classof() >> isn't used though. > > Curious. Seems that might be worth some further thought. > >> >> 0007-Remove-pointless-classof-s.patch >> Mass removal of useless classof()'s. >> >> Comments and review welcome. >> >> -- Sean Silva >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>
die-on-upcast.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
