[PATCH] D98477: [ADT] Add IntrusiveVariant

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Thank you for taking a look @whisperity! I'm glad there is interested from others in the new type, I was worried at first it may be too niche of a use.

I didn't notice your comments until late today, but next week I will make a pass over them, respond, and update the patch.

I had intended to add more potentially-interested reviewers, possibly by posting to llvm-dev, but I was also in the process of rebasing the patchset which precipitated me factoring this out into `include/ADT` and wanted to make sure it was at the very least usable in that context, so thank you for adding some!

I'm also leaving a few comments I had already drafted as a reminder to myself.



================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:215
+  UnionImpl(const UnionImpl &) = default;
+  UnionImpl(UnionImpl &) = default;
+  ~UnionImpl() = default;
----------------
Should be a move constructor?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:257
+template <std::size_t Index, typename VisitorT, typename... VariantTs>
+struct TrappingIntrusiveVariantVisitor {
+  // We assume we are used in a context where either:
----------------
Maybe rename to `FallibleIntrusiveVariantVisitor` and `fallibleVisit` or `visitOrError`?


================
Comment at: llvm/unittests/ADT/IntrusiveVariantTest.cpp:27
+  IntrusiveVariant<A, B> V;
+  visit(makeVisitor([](A) {}, [](B) { FAIL(); }), V);
+  visit(makeVisitor([](B) { FAIL(); }, [](A) {}), V);
----------------
These could be simplified, e.g. `ASSERT_TRUE(V.hasAlternative<A>())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98477