[PATCH] D125609: [ADT] Adopt the new casting infrastructure for PointerUnion

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman added a comment.

In D125609#3514009 <https://reviews.llvm.org/D125609#3514009>, @0x59616e wrote:

> Define `CastInfoPointerUnionImpl` as below :
>
>   template <typename... PTs> struct CastInfoPointerUnionImpl {
>     using From = PointerUnion<PTs...>;
>   
>     template <typename To> static inline bool isPossible(From &F) {
>      return  F.Val.getInt() == FirstIndexOfType<To, PTs...>::value;
>     }
>   
>     template <typename To> static To doCast(From &F) {
>       assert(isPossible<To>(F) && "cast to an incompatible type !");
>       return PointerLikeTypeTraits<To>::getFromVoidPointer(F.Val.getPointer());
>     }
>   };
>
> and declared it as a friend of PointerUnion.

Could you declare the CastInfo struct itself as a friend? something like `template<typename To, typename...PTs> friend struct CastInfo<To, PointerUnion<PTs...>>`? That way you don't need the impl struct so it cuts down on overall code. Truly asking here - I think it's doable but I may be wrong.

> And then `CastInfo`:
>
>   template <typename To, typename... PTs>
>   struct CastInfo<To, PointerUnion<PTs...>>
>       : NullableValueCastFailed<To>,
>         DefaultDoCastIfPossible<To, PointerUnion<PTs...>,
>                                 CastInfo<To, PointerUnion<PTs...>>> {
>     using From = PointerUnion<PTs...>;
>     using Impl = CastInfoPointerUnionImpl<PTs...>;
>   
>     static inline bool isPossible(From &f) {
>       return Impl::template isPossible<To>(f);
>     }
>   
>     static To doCast(From &f) {
>       return Impl::template doCast<To>(f);
>     }
>   };

This looks really nice - I think it still will even with the Impl::* methods inlined :)

> By doing this we can implement `PointerUnion::is` and `PointerUnion::get` in this way:
>
>   /// Test if the Union currently holds the type matching T.
>    template <typename T> bool is() const { return isa<T>(*this); }
>   
>    /// Returns the value of the specified pointer type.
>    ///
>    /// If the specified pointer type is incorrect, assert.
>    template <typename T> T get() const {
>      assert(isa<T>(*this) && "Invalid accessor called");
>      return cast<T>(*this);
>    }
>
> Does this make sense ?

Comments inline :) Overall makes good sense to me though!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125609/new/

https://reviews.llvm.org/D125609