[ADT] Adopt the new casting infrastructure for PointerUnion

[bzcheeseman]

llvm/include/llvm/ADT/PointerUnion.h

213

CastIsPossible inheritance is not necessary since you define isPossible below.

216

Out of curiosity, does it make sense to implement PointerUnion::is<T> in terms of isa<T> rather than the other way around? And same for doCast below?

224

You can inherit from the NullableValueCastFailed cast trait to get this

226

You can inherit from the DefaultDoCastIfPossible cast trait to get this

[0x59616e]

Given that I'm still young to C++ and llvm, every suggestion, even the
one you feel it's trivial, are very welcomed ;)

This diff comes with the following updates:

• Define PointerUnion::is and PointerUnion::get in terms of isa<T> and cast<T>.
• Add FIXME comment for PointerUnion::is(), PointerUnion::get() and PointerUnion::dyn_cast().

[0x59616e]

llvm/include/llvm/ADT/PointerUnion.h

216

I'm feeling the same, too.

[0x59616e]

In D125609#3513978, @0x59616e wrote:

Given that I'm still young to C++ and llvm, every suggestion, even the
one you feel it's trivial, are very welcomed ;)

This diff comes with the following updates:

• Define PointerUnion::is and ::get in terms of isa<T> and cast<T>.
• Add FIXME comment for is(), get() and dyn_cast().

Also, CastInfoPointerUnionImpl is defined and declared as a friend of PointerUnion to support isa<T> and cast<T> directly rather than calling into PointerUnion::is and PointerUnion::get.

[0x59616e]

Didn't notice the build failure. Remove the CastInfoPointerUnionImpl and revert
back to the original implementation.

Is there any way for CastInfo to directly access PointerUnion::Val, which is
a protected member ?

[0x59616e]

I can only come up with this :

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.

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);
  }
};

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 ?

[bzcheeseman]

In 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!

[bzcheeseman]

llvm/include/llvm/ADT/PointerUnion.h

213

I think you'll need public inheritance here and below? Otherwise I'd expect that you get compiler errors saying this partial specialization doesn't define the functions you need.

[0x59616e]

Implement CastInfoPointerUnionImpl

[0x59616e]

Thanks for feedback ;)

In D125609#3514018, @bzcheeseman wrote:

In 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.

It seems that partial specialization struct can't be declared as a friend ?

[0x59616e]

I used arc incorrectly. Re-update the diff.

[bzcheeseman]

In D125609#3514030, @0x59616e wrote:

Thanks for feedback ;)
<snip>
It seems that partial specialization struct can't be declared as a friend.

:( I know you can have a templated friend declaration but I guess this doesn't work.

In that case this approach seems fine, thanks for working on it! Just a few more cleanups but overall I think it's looking really solid :)

llvm/include/llvm/ADT/PointerUnion.h

91

nit: Full sentences, please.

128

A quick comment to explain what's going on would be great, maybe /// This is needed to give the CastInfo implementation below access to protected members. or something like that? Just a quick one-liner so it's not confusing in the future.

210

Please explain why this struct is needed with a /// comment.

243

Public inheritance here too. Do you have checks that const is handled correctly?

[0x59616e]

Thanks for feedback !

• Add test case for const
• Add some comments.

[0x59616e]

llvm/include/llvm/ADT/PointerUnion.h

243

I've added some test for const case, but I'm not sure whether it's comprehensive enough or not.

[0x59616e]

• public inheritance for ConstStrippingForwardingCast

[bzcheeseman]

llvm/include/llvm/ADT/PointerUnion.h

243

If you see the tests for the cast infra there are some spots where we have auto result = cast<T>(something); followed by a static_assert that decltype(result) must be a specific type. That was the kind of thing I meant here - i.e. if I have a const pointer union and I do cast<float *> (for example), should it return const float *? Or not? I guess what I was going for was to explicitly test "what happens when I have a const input to a cast? How explicit do I have to be?

At the bottom there you're really close - it looks like you're on the right track but it would be great to get a static_assert for what the output type of a cast on PU4 is, just to make it explicit.

[bzcheeseman]

This comment was removed by bzcheeseman.

[0x59616e]

llvm/include/llvm/ADT/PointerUnion.h

243

What's your opinion about whether it should return a const or non-const pointer when casting from a const PointerUnion ?

[0x59616e]

It seems that people are not always using a plain pointer in PointerUnion. Sometimes they seem to use a custom smart pointer ?

[0x59616e]

update diff

• define our own castFailed in the hope of fixing the build failure of clang.
• add static_assert for type check

[0x59616e]

update diff trying to fix buildbot failure

[0x59616e]

apply clang-format

[bzcheeseman]

Thank you for working on this!

llvm/include/llvm/ADT/PointerUnion.h

243

Honestly I'm not sure! I think it comes down to the semantics of PointerUnion, sort of like const void const* where each const means slightly different things. I think the way that seems simplest is to say "yes, const comes out of a const union". The way that I think matches C++'s const rules is "no, const applies to the union, not the things inside".

I think in this patch you're using the second way, which is totally fair (and you added tests to make it explicit, so thank you!)

llvm/unittests/ADT/PointerUnionTest.cpp

283 ↗

(On Diff #429526)

misspelling here, "misamatch" -> "mismatch"

[lattner]

I'm excited to see this, thank you or working on it. +1 with the changes aman suggested.

[0x59616e]

fix typo

[0x59616e]

Thanks @lattner @bzcheeseman ! I'll land this patch as soon as possible after making sure everything runs smoothly.

llvm/include/llvm/ADT/PointerUnion.h

Derived &operator=(Type V) {
       this->Val = ValTy(
           const_cast<void *>(PointerLikeTypeTraits<Type>::getAsVoidPointer(V)),
           I);
       return static_cast<Derived &>(*this);
     };
   };
 }
 
 // forward declaration of CastInfoPointerUnionImpl
 template <typename... PTs> struct CastInfoPointerUnionImpl;
 /// A discriminated union of two or more pointer types, with the discriminator
 /// in the low bit of the pointer.
 ///
 /// This implementation is extremely efficient in space due to leveraging the
 /// low bits of the pointer, while exposing a natural and type-safe API.
 ///
 /// Common use patterns would be something like this:
class PointerUnion
   // The first type is special because we want to directly cast a pointer to a
   // default-initialized union to a pointer to the first type. But we don't
   // want PointerUnion to be a 'template <typename First, typename ...Rest>'
   // because it's much more convenient to have a name for the whole pack. So
   // split off the first type here.
   using First = TypeAtIndex<0, PTs...>;
   using Base = typename PointerUnion::PointerUnionMembers;
 
   friend struct CastInfoPointerUnionImpl<PTs...>;
 
 public:
   PointerUnion() = default;
 
   PointerUnion(std::nullptr_t) : PointerUnion() {}
   using Base::Base;
 
   /// Test if the pointer held in the union is null, regardless of
   /// which type it is.
   bool isNull() const { return !this->Val.getPointer(); }
 
   explicit operator bool() const { return !isNull(); }
 
+  // FIXME: Replace the uses of is(), get() and dyn_cast() with
+  //        isa<T>, cast<T> and the llvm::dyn_cast<T>
+
   /// Test if the Union currently holds the type matching T.
   template <typename T> inline 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> inline T get() const {
     assert(isa<T>(*this) && "Invalid accessor called");
bool operator!=(PointerUnion<PTs...> lhs, PointerUnion<PTs...> rhs) {
   return lhs.getOpaqueValue() != rhs.getOpaqueValue();
 }
 
 template <typename ...PTs>
 bool operator<(PointerUnion<PTs...> lhs, PointerUnion<PTs...> rhs) {
   return lhs.getOpaqueValue() < rhs.getOpaqueValue();
 }
 
 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());
   }
 };
 
 // Specialization of CastInfo for PointerUnion
 template <typename To, typename... PTs>
 struct CastInfo<To, PointerUnion<PTs...>>
     : public NullableValueCastFailed<To>,
       public 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);
   }
 };
 
 template <typename To, typename... PTs>
 struct CastInfo<To, const PointerUnion<PTs...>>
     : ConstStrippingForwardingCast<To, const PointerUnion<PTs...>,
                                    CastInfo<To, PointerUnion<PTs...>>> {};
 
 // Teach SmallPtrSet that PointerUnion is "basically a pointer", that has
 // # low bits available = min(PT1bits,PT2bits)-1.
 template <typename ...PTs>
 struct PointerLikeTypeTraits<PointerUnion<PTs...>> {
   static inline void *getAsVoidPointer(const PointerUnion<PTs...> &P) {
     return P.getOpaqueValue();