[boost] [variant2] Formal review

Subject: [boost] [variant2] Formal review
From: Bjorn Reese (breese_at_[hidden])
Date: 2019-04-13 17:26:00

I. DESIGN
---------

The main design decision of Variant2 is the never-empty guarantee, which
is relevant when replacing the value of a variant. This is done without
overhead if the alternative types are "well-behaving"; if they are not,
then Variant2 falls back to using double storage.

This is a major difference to:

   * std::variant which enters an invalid (valuless_by_exception) state.
     While that may be appropriate for some use cases, there are others
     where it is not. For instance, the variant may be used for a state
     machine where the alternative types are function object types to be
     invoked when events arrive. Having a valueless state is the wrong
     choice for this use case.

   * Boost.Variant which uses a temporary heap object instead. Avoiding
     this internal heap allocation is sufficient grounds for another
     never-empty variant type. In time critical applications heap
     allocation not only introduces indeterministic timing, but may also
     incur priority inversion due to thread synchronization within the
     heap allocator.

Variant2 has several ways to ensure the never-empty guarantee:

   V1. If all alternative types have non-throwing assignment, the variant
       cannot fail during re-assignment.

   V2. If an explicit null state is available, then this is chosen on
       re-assignment error. This is similar to std::variant, except is
       is opt-in.

   V3. If one of the alternative types has a non-throwing default
       constructor, then this is chosen on re-assignment failure.

   V4. Otherwise, the variant will remain it its old state (due to double
       storage) if re-assignment fails.

Criterion V3 is possibly surprising because the variant may change to
another type not anticipated by the user. This should be opt-in instead
(e.g. using a tag.)

I am missing a function to get the index of a type to work with the
index() function. Something similar to holds_alternative() but that
returns the index instead of a boolean. This useful in switch
statements, when we want to avoid visitors for some reason:

   variant<int, float> var = 1;

   switch (var.index()) {
     case index_of<int>(var): /* Do int stuff */ break;
     case index_of<float>(var): /* Do float stuff */ break;
   }

It would also be beneficial for performance reasons to add accessors
that have a narrow-contract, similar to relaxed_get() in Boost.Variant.
The implementation is already there (detail::unsafe_get.)

Better compiler error messages would be desirable. For instance, if we
use an illegal visitor that has different return types, then the
compiler error does not give us any hint about the different return
types.

II. IMPLEMENTATION
------------------

The implementation is high quality. Although it does lots of
meta-programming, most of the style is simple and easy to follow,
especially if you have a rudimentary knowledge of Boost.Mp11.
In a few places, like the visit() return type, the meta-programming
is more hardcore.

variant<T...> defines _get_impl as a public "private" function. This
can be made more safe by using detail::unsafe_get instead of _get_impl
throughout the code. detail::unsafe_get will continue to call _get_impl,
but the former should be made a friend of variant<T...> such that
_get_impl can be made private.

The other public "private" function, variant<T...>::_real_index(), does
not seem to be used and can be removed.

The emplace_impl(mp_false, mp_false, ...) contains two distinct cases
that depend on a compile time condition. If this is split in two, then
the two assert()s can be changed into static_assert()s.

The variant_alternative and get functions have a couple of compiler
workarounds. The code could be made clearer if the workaround is always
used for all compilers. Or does is this related to C++11 constexpr?

III. DOCUMENTATION
------------------

The documentation is mainly aimed at experienced users.

There is no tutorial for people unfamiliar with variant types, nor is
there a design rationale that explains why Variant2 is exists when we
already have Boost.Variant and std::variant.

The reference documentation is adequate, although a bit terse.

IV. MISC
--------

Having written my own variant class (that has a maximum size, and any
type exceeding the limit is allocated on heap) puts me in a good
position to assess Variant2. I was also the review manager of Boost.Mp11
upon which Variant2 is built.

I have spent 5-6 hours reading documentation, reviewing the code, and
writing small test programs.

V. VERDICT
----------

Variant2 fills a hole not covered by Boost.Variant and std::variant.

I vote to CONDITIONALLY ACCEPT Variant2 into Boost, provided that the
following change be made:

   1. The selection of an arbitrary non-throwing type in case of an
      re-assignment failure (criterion V3 in the DESIGN section above)
      should be made optional. By default it should use double storage
      in this case.

I would furthermore like to see improvements to the documentation, as
well as the index_of/safe_get functionality, but neither is a
requirement for acceptance.

I have not reviewed expected<T, E...>.