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

[Chris Lattner]

On Oct 10, 2012, at 2:53 PM, Sean Silva <silvas at purdue.edu> wrote:

> Ping.
> 
> Chris, could you take a look at this?

I don't think I have any special value to be added here.  Some thoughts though: I think it *does* make sense to allow cast<>'s that obviously constant fold to true or false from static type information, this does help generic code.  Also, the bugs that disallowing it would find are pretty easy to find through other means.

The patches look fine to me.  I'm not hugely thrilled with Casting.h pulling in type_traits to get enable_if, but I don't have a better idea. :)  Please commit,

-Chris

> 
> -- Sean Silva
> 
> On Sun, Oct 7, 2012 at 5: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. It also simplifies implementing LLVM-style RTTI since now one
>> fewer classof() is required.
>> 
>> 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.
>> 
>> 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