Boost logo

Boost :

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.


- 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
- Docs for expected are completely missing.


- 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.


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.


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).

Boost list run by bdawes at, gregod at, cpdaniel at, john at