[PATCH] D99561: Support visitor pattern by PointerUnion.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: lhames.
dblaikie added a comment.

In D99561#2660015 <https://reviews.llvm.org/D99561#2660015>, @bakhtiyarneyman wrote:

> Thanks for taking a look, Mehdi and David! PTAL.
>
> David, it was a goal to try to keep the API similar to std::variant. The biggest difference is that std::visit is a static function that accepts variable number of variants. I never experienced a need for that and replaced it with a non-static method (less verbose, more idiomatic, less complex). In my mind this API difference is insignificant, and the APIs are similar enough, but maybe when you say similar you mean something different?

I'd be inclined to stick with the same API as std::variant - though I guess that API doesn't directly make sense/isn't especially usable until C++23 with overloadable lambdas, etc. Fair enough, member seems OK, I'd lean slightly towards a bit more std::variant like (eg: `llvm::visit(lambda_1, lambda_2, ..., the_variant);` - yeah, it does feel a bit wonky, but at least for now I'd favor the consistency even with a future standard API - but not so much that I'll insist on it - up to your judgment).

(+ @lhames too here, in case he's interested in this sort of thing)



================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:69-91
+  template <std::size_t I, class... T> struct SelectTypeAtIndex {};
+
+  template <std::size_t I, class T0, class... Tn>
+  struct SelectTypeAtIndex<I, T0, Tn...> : SelectTypeAtIndex<I - 1, Tn...> {};
+
+  template <class T0, class... Ts> struct SelectTypeAtIndex<0, T0, Ts...> {
+    using type = T0;
----------------
sorry - could you summarize what is changing here?


================
Comment at: llvm/include/llvm/ADT/PointerUnion.h:279-299
+    static_assert(
+        sizeof...(PTs) <= 8,
+        "PointerUnion::visit implemented for at most 8 alternatives.");
+    switch (this->Val.getInt()) {
+    case 0:
+      return visitor(Getter<0>::get(*this));
+    case 1:
----------------
No chance of reasonably generalizing this? Pity to have such a hardcoded limit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99561