[llvm-commits] [cfe-commits] Casting.h: Automatically detect isa<Base>(Derived).

[Sean Silva]

>> 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 <dblaikie at gmail.com> wrote:
> On Sun, Oct 7, 2012 at 2:04 PM, Sean Silva <silvas at purdue.edu> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: die-on-upcast.patch
Type: application/octet-stream
Size: 1342 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121007/4d98fe9f/attachment.obj>