[boost] [variant2] Review

Subject: [boost] [variant2] Review
From: Jan Herrmann (jherrmann79_at_[hidden])
Date: 2019-04-15 21:14:34

Hello,

I looked at the review, at the code and some time ago I tried
to implement my own variant. In my opinion in variant2 the
individual parts are more worth than the whole. Most
implementation details would be of great use for implementing
variants with a differnt trade-off. Hence the library ought
to be split into several independend parts:
* a typing switch
* a variadic union
* an extendable visitation mechanism
* a variant view
* related type traits
* several implementation of a concrete variant

1. A typing switch
The typing switch would be provide a litle bit more than (and
can be implementated with help of) mp_with_index.

The following pseudo code shows the implementation idea:

template<size_t MaxIndex, class Function, class Cast, class Object>
constexpr decltype(auto)
typing_switch(size_t index, Function fun, Cast cast, Object obj) {
     return fun(cast(integral_constant<size_t, index>{}, obj));
}

The object is a variant and the cast transfroms this special variant
to a concrete value (with type). The function is provided by the
variant user. The visit function (line 1732) would become something
like this:

template<class Variant>
struct variant_cast
{
     template<class Index>
     constexpr decltype (auto) operator()(Index, Variant& V) {
         return unsafe_get<Index::value, V>();
     }
};

template<class Fun, class Variant>
constexpr decltype(auto) visit( F&& f, V1&& v1 )
{
     return typing_switch<variant_size<V1>>(v.index(), f,
variant_cast<V1>, v1);
}

A simplified deserialisation can be implementated as follows:

template<class Types>
struct index_to_type
{
     template<class Index>
     constexpr decltype (auto) operator()(Index, std::nullptr_t) {
         return mp_identity<mp_at<Types, Index>>{};
     }
};

template<class Variant, class Archive>
Variant deserialize(Archive& ar) {
     size_t index;
     ar >> index;
     Variant f = [&ar](auto type) {
         decltype(type)::type concrete_element;
         ar >> concrete_element;
         return Variant{std::move(concrete_element)};
     };

     return typing_switch(index, f, index_to_type<Variant>{}, nullptr);
}

With help of this function an implementation of a visitation function
is easy.

2. A varidic union
Without doubt a type implementing a varidaic union is a
building block for almost every variant implementation.
It should be part of the public interface, well tested
and with a minimal interface (get<i> and emplace<i>)

3. An extendable visitation mechanism
This could be used to implement visitation on multiple
different variants. It should use visit on one variant as
customization point.

4. A variant view (and a mutable variant view)
It could have the following form:

template<class... T>
class variant_view {
     void* value;
     size_t index;
public:
     /* visit ... */
};

It is a little bit like folly/DiscriminatedPtr.h
(https://github.com/facebook/folly/blob/master/folly/DiscriminatedPtr.h)
but without platform specific code. It can be used for functions
which take variants as a const references (or in the mutable form
as variants which can change the value but not the actual type).
It can be constructed from corresponding variants, values (which
are part of T...) and other variant_views. As long as it is constructed
from a variant implemented with the variadic union the process can
be simple: all members of a union have the same address as the union
and the index is equal. For special cases the process can be more
complicated. For other variant_views the transformation can be
implemented as a runtime lookup in an array and for simple values it
is simple.
A variant_view can for example be used for functions accepting const
references like output and serialisation, or for referencing "polymorphic"
objects like nodes in ASTs.

5. Related type traits
Meaningfull type traits which can be used in one variant have a
good chance be be used in a related one. If one variant want to static
assert in case void is one of its types the logic is probably usefull
for others. They should be collected.

6. Several implementation of a concrete variant
This is the place for variant2.

With this building blocks it should be simpler to implement a custom
variant.
Now lets think of a few use cases:

A class contains 3 variants each containing only a few "small" types.
Instead of using 3 variants it could pack all indices into one int
and construct variant_views as needed. This can reduce the size of the
class and saves memory bandwidth. Editing of variants would be of course
more complicated.

A variant containing a few trivial small types which are used in allmost
all cases and one type (T_evil) which is used rarely, is big and can
throw in certain situation. The soloution can be a normal variant with
the exception of storing T_evil as a smart pointer and dereferences it
when visited.

A variant with allocator support. In case a user has a solution (or
idea) for their variant which lives in shared memory a simple blueprint
with generic components like varidic union, a typing switch and useable
type traits may be of help.

I think requirements for different variant types mentioned in the review
could be implemented with some well defined basic building blocks.

Beside these thougths I looked at the library.
expected.hpp:
I tried to compile the following file (containing only one non empty line):

#include <boost/variant2/expected.hpp>

and it does not compile. It should be a self-contained header. Therefore
it should be not part of the library.

variant.hpp:
Personally (in contrast to other reviewers) I would prefer code more
structured in different files. In an ideal world there is for every
component a single header file (in clean C++ 17) with minimal dependencies
which directes  for compiler (and version) specific stuff to a different
header. For the future it would result in source code written in newest
standard without disturbing influence from ifdefs caused by old buggy
compilers.

The source has more than 1800 lines with only a few comments, short (1
char) names and very long lines (200 chars and more). For me it is not
easy to read (after clang-format it is 2030 lines). Variant alone has
more than 450 lines of code. With more shorter files the connection of
(and dependencies among) the components would be more visble.

I started at the begining of the file and searched for a testcases for
monostate and I didn't found one. There are a few testcases inside
variant_valueless.cpp which use monostate but none which test it
directly. Next variant_size. There are testcases but most deal with
variants of type 'variant<> const', 'variant<void>' and the like.
Declaring variables of these types results in compile time errors. It is
not wrong std::variant gave me the same result, but it is a kind of
strange that most tests are implemented in terms of variants wich are
ill-formed. On the other hand there are no tests which do not compile.
These are two example which should not compile.
The template detail::variant_alternative_impl looks like something which
copies const, volatile and references. Is there a difference to
copy_cv_ref at the end? Could it be used?
At the end of the file the comparison is implemented. But operator!= is
not implementated in terms of operator== and operator<= and others not
in terms of operator<. In case there is a reason for it documentation
would be nice.

Summarized I think variant2 should NOT be part of boost. I think the
focus should be more on reusable components and better documented code.
I think most of the code could be very usefull with more descriptive
names and test.

Regards Jan Herrmann