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

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman added a comment.

In D125609#3514030 <https://reviews.llvm.org/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 :)



================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:91
 
+// forward declaration of CastInfoPointerUnionImpl
+template <typename... PTs> struct CastInfoPointerUnionImpl;
----------------
nit: Full sentences, please.


================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:128
 
+  friend struct CastInfoPointerUnionImpl<PTs...>;
+
----------------
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.


================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:210
 
+template <typename... PTs> struct CastInfoPointerUnionImpl {
+  using From = PointerUnion<PTs...>;
----------------
Please explain why this struct is needed with a `///` comment.


================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:243
+struct CastInfo<To, const PointerUnion<PTs...>>
+    : ConstStrippingForwardingCast<To, const PointerUnion<PTs...>,
+                                   CastInfo<To, PointerUnion<PTs...>>> {};
----------------
Public inheritance here too. Do you have checks that `const` is handled correctly?


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

https://reviews.llvm.org/D125609