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

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman accepted this revision.
bzcheeseman added a comment.
This revision is now accepted and ready to land.

Thank you for working on this!



================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:243
+struct CastInfo<To, const PointerUnion<PTs...>>
+    : ConstStrippingForwardingCast<To, const PointerUnion<PTs...>,
+                                   CastInfo<To, PointerUnion<PTs...>>> {};
----------------
0x59616e wrote:
> bzcheeseman wrote:
> > 0x59616e wrote:
> > > bzcheeseman wrote:
> > > > Public inheritance here too. Do you have checks that `const` is handled correctly?
> > > I've added some test for const case, but I'm not sure whether it's comprehensive enough or not.
> > 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.
> What's your opinion about whether it should return a const or non-const pointer when casting from a const `PointerUnion` ?
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!)


================
Comment at: llvm/unittests/ADT/PointerUnionTest.cpp:283
+  static_assert(std::is_same<double *, decltype(result1)>::value,
+                "type misamatch for cast with PointerUnion");
+
----------------
misspelling here, "misamatch" -> "mismatch"


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

https://reviews.llvm.org/D125609