[boost] [variant2] Review of Variant2

Subject: [boost] [variant2] Review of Variant2
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-04-14 15:00:46

To never-empty or not to never-empty (a.k.a. Design):

In general, since we already have std::variant with the valueless state, 
having an alternative that provides never empty guarantee is useful if 
only to gain practical experience with both approaches. Again, in 
general, I consider never empty guarantee a plus as it reduces the total 
number of states of a program, making it easier to reason about and 
removing performance overhead.

However, I agree with Andrzej in that the behavior when after an 
exception a seemingly random value appears in the variant out of the 
thin air is not useful and may even be harmful. I realize that there are 
cases when one simply doesn't care about the variant contents after an 
exception, but still that kind of uncertainty seems unneeded and 
undesired to me. Frankly, in my experience, people (myself included) 
often simply forget or don't consider what will happen if assignment 
fails, leaving subtle bugs in the code. I would very much prefer to be 
explicit about when I allow a different value to appear in the variant. 
I think, including monostate in the list of types would be an adequate 
way of telling that intention. Meaning that when monostate is *not* 
present, I would not expect any funny business from the variant, and 
therefore the variant has to use double storage, if it must.

It's disappointing that Boost.Variant2 does not depart from the existing 
Boost.Variant in this regard, which kind of diminishes its value. I'm 
not saying Boost.Variant design is bad, I'm saying that we already have 
it, and a potential new library could offer something different, 
something not supported by either std::variant or Boost.Variant, even if 
as an option. I guess, there is some value in facelifting Boost.Variant 
to the more recent C++ practices and interface closer to std::variant, 
which makes learning easier.

Docs:

- In general, the docs are fairly minimal. I think, it could benefit 
from more rationale discussion and guidelines for unexperienced users, 
like when to use std::variant, Boost.Variant and Boost.Variant2. I 
think, the very Boost.Variant2 (and the variant class itself) naming is 
unfortunate in this regard, as it could be a hint in itself. There's not 
a single usage example in the docs. I think, in the current form docs 
are sufficient for experienced programmers who have used some version of 
variant in the past, but not so much for new users who are picking their 
first variant ever. Some performance numbers would be useful as well, 
although not a requirement.
- One important piece of information that is missing in the docs is what 
is the behavior if only the second bullet in the Overview is satisfied 
and assignment/emplace throws. Specifically, what is the resulting value 
of the variant in this case.
- List of supported compilers and minimum C++ version is missing.
- It would be nice to mention whether the double storage is static (i.e. 
it always contributes to the variant size) or dynamic.
- In the synopsys, use std::size_t vs. size_t consistently.
- In variant reference description, define that T0 is the first element 
of T argument pack.
- The use of Boost.Mp11 in the deference description requires knowledge 
of Boost.Mp11, which is unfortunate. It's probably better to use 
standard C++ (e.g. C++17 folding expressions, even though the library 
itself might be C++11) or plain English.
- I don't understand the description of the "variant(U&& u)" 
constructor. "build an imaginary function FUN(Ti) for each alternative 
type Ti" - what function, with what effect? Are you trying to say that 
the resulting type of the stored value is selected as-if by C++ overload 
resolution? I think, it should be phrased better.
- In variant_alternative description, move the variant_alternative<I, 
variant<T...>> specialization description to the beginning so that the 
other specializations on the various qualified Ts use the knowledge 
about what variant_alternative actually does and that T is supposed to 
be a variant.
- In variant relational operators description, w.index - parenthesis 
missing.
- Docs for expected are completely missing.

API:

- Why have valueless_by_exception() if it's never true? You're not 
giving a rationale in the docs. Trying to follow std::variant interface 
in this part is not a convincing argument because the never empty 
guarantee *is* the defining difference between the two. Someone who 
conciously chose variant2 will never want to check if it's valueless.
- I think, a function indicating whether subset() is possible (i.e. the 
variant contains a value representable by the subset) would be useful. 
This would allow to avoid dealing with exception from subset().
- Moving subset() overloads should probably be 
noexcept(std::is_nothrow_move_constructible_v<T> &&...).
- Given that there are no docs and next to no comments in the code, I'm 
not able to comment on expected.

Implementation:

I didn't pay much attention to the implementation, but I trust Peter's 
code is excellently written.

Looking at expected implementation, it seems to be using variant through 
its public interfaces, so it should be possible to extract it as a 
separate library.

My knowledge of the domain:

I have experience using both Boost.Variant and std::variant. I'm not an 
often user of these components, though.

My reviewing effort:

A few hours reading the docs and glancing over the code. I didn't try 
compiling the code.

Verdict:

Given that I see Boost.Variant2 as a more modern equivalent to 
Boost.Variant, I would expect it to replace Boost.Variant over time. 
With that in mind, I vote to CONDITIONALLY ACCEPT the library, with the 
following conditions:

- Significantly improve documentation. Specifically, provide sections 
discussing corner cases regarding state of the variant after exceptions, 
provide usage examples and a tutorial, provide design rationale. 
Ideally, add a migration guide from Boost.Variant. I'd like to see 
something closer to Boost.Variant docs.
- Remove expected. It's not documented, it's not related to the variant 
implementation and it can totally be implemented as a separate library.

Beside these mandatory requests, I'd be very interested if Peter 
presented an updated variant proposal, which doesn't create values out 
of thin air except if it's monostate. I know it's not what is being 
proposed now as Boost.Variant2, but had it been proposed, I would've 
been more interested in that proposal than this one. If Peter finds a 
way to incorporate this into Boost.Variant2 (e.g. as a policy or a 
separate variant type), I'd be happy as well.

Lastly, thank you Peter for submitting this library for review and thank 
you Michael for managing the review (especially, for extending the 
review period).