[PATCH] D98477: [ADT] Add IntrusiveVariant

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D98477#2664061 <https://reviews.llvm.org/D98477#2664061>, @scott.linder wrote:

> In D98477#2660486 <https://reviews.llvm.org/D98477#2660486>, @dblaikie wrote:
>
>> Perhaps PointerUnion could be implemented as a derivation from this intrusive variant in a follow-up change, subsuming D99561 <https://reviews.llvm.org/D99561>
>
> I'm not as familiar with the implementation of `PointerUnion`, I just noted when I did a pass over the LLVM container types that it wouldn't be sufficient for the specific thing I was working on, so I started writing my own compact variant-like type. At the very least I think we should have a common `llvm::visit` with specializations for both `llvm::PointerUnion` and `llvm::IntrusiveVariant`.

Hopefully/probably overloads, rather than specializations (it's not possible to partially specialize a function template, in any case), but yeah -



================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:159-166
+/// A helper macro to add the declarations needed to use a type as an
+/// alternative for IntrusiveVariant. Must be the first declaration of the
+/// class.
+#define DECLARE_INTRUSIVE_ALTERNATIVE                                          \
+  ::llvm::IntrusiveVariantTag IntrusiveVariantTagMember;                       \
+  template <typename...> friend class ::llvm::IntrusiveVariant;                \
+  template <std::size_t, typename, typename...>                                \
----------------
Can we instead use/generalize something like the type traits used for PointerUnion that declare the number of spare bits in the type? Since we already have those in LLVM (& to macro-y).


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:276-280
+// case limbs still have to resolve to some overload, but we won't have an
+// overload available for the cases where the index is too large (i.e. we want
+// there to be no such overload, so we get compile-time errors for those cases
+// everywhere else). As a workaround, we make the body of the out-of-bounds
+// overloads be llvm_unreachable.
----------------
Not sure I follow why this ends up as a runtime error - what if the overload with unreachable (down at 307) wasn't defined? Wouldn't that produce a compile-time error only when the index is out of bounds?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:327-343
+// Macro for writing the 32-case switch statement from a definition of CASE(N)
+#define CASES8(B)                                                              \
+  CASE((B + 0))                                                                \
+  CASE((B + 1))                                                                \
+  CASE((B + 2))                                                                \
+  CASE((B + 3))                                                                \
+  CASE((B + 4))                                                                \
----------------
There's a fair bit of macro expansion stuff here - any chance it can be restructured to use variadic templates to generalize better/reduce the macro usage?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:429-432
+  union {
+    detail::CommonInitialSequenceT CommonInitialSequence;
+    UnionT Union;
+  };
----------------
Technically C++ doesn't allow access to a common initial sequence except for types with standard layout, I think? So types with non-trivial copy ops, etc, wouldn't be usable here. (I wonder what PointerUnion does? I guess it just stuffs everything in void* or uintptr_t, which isn't exactly applicable here - but maybe some aligned storage would be suitable here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98477