[PATCH] D98477: [ADT] Add IntrusiveVariant

[Whisperity via Phabricator via llvm-commits]

whisperity added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:12
+// union members to store the runtime tag at the beginning of the variant
+// alternative itself, allowing for it to be packed more efficiently into bits
+// that would otherwise be used for padding.
----------------
> Itself
Who? The `IntrusiveVariant` or the union members?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:35-36
+//  * The set of alternative types must be unique, and cannot be referred
+//    to by index (although internally an index is materialized to support
+//    efficient dispatch in llvm::visit).
+//  * There is a static limit on the number of alternative types supported,
----------------
I think the implementation detail in the parens could go somewhere else. If this little bit of trivia shouldn't interest the average client library, it shouldn't be placed into an obvious position.


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:36-38
+//    efficient dispatch in llvm::visit).
+//  * There is a static limit on the number of alternative types supported,
+//    as the dispatch in llvm::Visit is switch-based. The idea is that the
----------------
So is it `visit` or `Visit`?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:37
+//    efficient dispatch in llvm::visit).
+//  * There is a static limit on the number of alternative types supported,
+//    as the dispatch in llvm::Visit is switch-based. The idea is that the
----------------
How much is the limit. Where is it stored? Might need a mention of the actual value (or symbol name) here.


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:54
+//  public:
+//    AltInt(int Int) : Int(Int) {}
+//    int getInt() const { return Int; }
----------------
Is it possible to suggest here writing an implicit conversion operator (`operator int() const { return Int; }`) and an assignment operator (`AltInt& operator=(int Int)`) to the wrapped type? Writing

```
if (V.holdsAlternative<AltDouble>())
{
  AltDouble AD = V.get<AltDouble>();
  takes_a_double(AD.getDouble());
  AD.setDouble(8.f);
}
```

is just incredibly superfluous when `std::variant` does this already with the `get<>` and the assignment operator.

I'm more concerned about the assignment operator. Can we assign a (mutable) reference to the variant's alternative?


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:59-67
+//  class AltDouble {
+//    DECLARE_INTRUSIVE_ALTERNATIVE
+//    double Double;
+//
+//  public:
+//    AltDouble(double Double) : Double(Double) {}
+//    double getDouble() const { return Double; }
----------------
I honestly believe for usability perspectives we should offer the user the option not to write all this out by hand. Just a simple wrapper over a double is 9 lines (or 11 if we include the assignment and implicit conversion from the suggestion above).

This is just waaaaaay too much. If we have like 2 or 3 variations in the variant, we are not much better (if any) than writing out the enum+union ourselves.

At least please consider providing a simple macro that wraps any alternate type into such a variant-capable thing.

For example, I'd like to use a variant like as follows. This is how one would use `(std|boost)::variant`, right?

```
struct Something { /* 3 data members */ };
struct SomeOtherThing { /* 3 other data members of different type! */ };


Variant<Something, SomeOtherThing> ThisOrThat;
```

Now I understand this doesn't work in your case directly. The whole intrusiveness would mean that I have to deal with either making the initial data member private, and then what happens to my other data members that should (or, must!) be public instead?

As a user, I am happy to put up with having to write two additional lines, i.e.

```
struct Something { /* 3 data members */ };
struct SomeOtherThing { /* 3 other data members of different type! */ };

MAKE_VARIANT_ALTERNATIVE_TYPE(Something);
MAKE_VARIANT_ALTERNATIVE_TYPE(SomeOtherThing);

Variant<AltSomething, AltSomeOtherThing> ThisOrThat;
//      ^~~           ^~~  Might be tricky to figure out!
```

But the idea is that //some// of the boilerplate required to make my types (or wrappers thereof) compatible with a working variant (which I love the idea of, by the way!) should not require me to write almost the same insane amount of code as I would have to with a conventional tagged union.



================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:138
+/// Simple value type which must be the first member of all alternative types
+/// of an IntrusiveVariant. See MAKE_INTRUSIVE_ALTERNATIVE.
+///
----------------
The referred macro(?) is not defined anywhere.


================
Comment at: llvm/include/llvm/ADT/IntrusiveVariant.h:144-145
+struct IntrusiveTag {
+  uint8_t Index;
+  IntrusiveTag() {}
+  IntrusiveTag(uint8_t Index) : Index(Index) {}
----------------
Are instances of this class only instantiated in the global scope? What's the guarantee that `Index` will initialise to anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98477