[boost] Variant2 review

Subject: [boost] Variant2 review
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2019-04-07 08:51:41

I'll firstly apologise for the brevity of this review. It is just before
the ACCU conference where I am speaking on a topic I have little
knowledge of, and I have done almost no prep due to insufficient hours
in the day. I am aware that if I don't make a review now, then I won't
at all, so this will have to do.

1. Overall I like the library, it adds significant benefit (primarily
removal of dynamic memory allocation) over Boost.Variant.

2. I don't like the lack of propagation of triviality of each individual
special member functions. std::variant does this. std::expected does
this. I can see Peter's point that for long lists of alternative states,
implementing this is a lot of pain for almost no gain.

However I do NOT agree with him for Expected. For a two-state variant,
it is very highly likely that triviality of special member function will
be very important for not a small number of use cases. I personally am
happy for Variant2 to implement Expected without review approval here IF
that Expected matches exactly std::expected. I am NOT happy if
Variant2's Expected breaks conformance on triviality of special member
function. If that is the only option on the table, then either please do
not include an Expected implementation, or don't include Variant2.

This raises a wider point though. What is good for a two-state variant
still applies, though less so, to three-state variants, four-state
variants and so on. I think there is a fundamental design issue here. If
Variant2 can't propagate triviality of individual special member
functions for small-number-of-state variants, then there is something
fundamentally wrong here.

3. I find the design choice to randomly permute state to avoid an empty
state baffling when there is a double buffer based design choice
available. I do not like, one bit, the random permutation of state. I
think it will lead to buggy code with hard-to-test corner cases. I think
it needs to go away in the proposed design.

I also don't get the dislike of the double buffer based design. Peter
seems to really dislike it, and avoids it whenever possible. I can't
understand why. If you can employ a double buffer design in order to
eliminate random permutation of state, to me this is an unalloyed good
in every way.

Nowhere does it say that a variant must be no larger than the largest of
its possible states. There is nothing wrong, *whatsoever*, with a
variant which is sized the largest of its possible states multiplied by two.

So please change the design to that instead. That should be the default
design, and design claims. Programmers should write code assuming that
design. IF it can be proven that a double buffer is unnecessary e.g. all
alternative types are noexcept, a monostate alternative is available
etc, THEN you can employ a single buffer implementation as an
*optimisation*.

That makes thing simple for the end user: add monostate, get single
buffer sized. Otherwise assume double buffer sized.

With a default double buffer design, most of the other issues which
other reviewers have raised go away. Even Robert's concerns about
assignment operators, with a double-buffer-by-default design one can
always implement the strong guarantee.

I badly need to depart, so I'll have to end there by saying that I vote
to ACCEPT, conditional on (i) propagation of triviality of special
member functions for at least small numbers of alternative states and
(ii) removal of ANY POSSIBILITY of random permutation of chosen state
under ANY circumstances.

Finally, thanks Peter for bringing this library for review. Nice library.

Niall