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

[David Blaikie]

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
>