[boost] [variant2] Andrzej's review -- documentation

Subject: [boost] [variant2] Andrzej's review -- documentation
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-04-07 00:18:08

Hi Everyone,
This is the review of the documentation of variant2.

First, the docs assume that the user is already familiar with std::variant
or boost::variant, and the introductory part only highlights the difference
from std::variant. It does not even mention that variant<X, Y> always
stores either X or Y. "Type-safe discriminated union (variant) type,"
sounds arcane to novices. Notation variant<T…> is also confusing to someone
who encounters variant for the first time.

There is no initial example. No mention of visit() which is the most
important part of the interface. Compare this with Boost.Variant docs:
https://www.boost.org/doc/libs/1_69_0/doc/html/variant.html

I do not think the library should assume this. Boost libraries are
characteristic of documentation that introduces potential users to the
problem domain and gives a sort of tutorial that shows how to use the
library. I recommend doing the same for variant2.

This library does a lot of contortions to avoid the valueless state, but
does not describe what the motivation for it is.

The docs also do not give a sense of what performance (spacial and runtime)
trade offs this library does. For instance, we do not know if the
double-buffer scenario th second buffer is within variant's storage or on
the heap. Or we do not know about secret moves that the library does
internally, which gives surprising behavior to the users that store
non-movable types in variant. Consider the following example:

```
struct X {};

struct Guard {
    explicit Guard(int) {}
    Guard(Guard&&) = delete;
};

variant<X, Guard> v {};
v.emplace<Guard>(1);
```

This works fine with std::variant, but fails to compile with
variant2::variant, because the latter requires all the alternative types,
at least in one implementaiton of the never-empty trick, to be
move-constructible. But this requirement is never listed in either global
requirements for alternative types stored in variant, or in the
specification of functions `emplace()`. Here's a wandbox example:
https://wandbox.org/permlink/hGbRtgzpUrpF0ODy

The Reference part of the docs is really good and detailed.

Some minor "bug reports" follow.

In the specification of functions types `T0` and `Ti` are used, but `T0` or
`Ti` are never defined. Wording in the C++ Standard also refers to `T0` and
`Ti`, but also provides a definition:

In the descriptions that follow, let i be in the range [0, sizeof...(Types)),
> and Ti be the ith type in Types.
> <http://eel.is/c++draft/variant.ctor#1.sentence-1>
>

See http://eel.is/c++draft/variant.ctor#1 A similar definition should be
put in variant2's documentation.

Noexcept specifications use mp_all. this assumes that users of variant2
must be familiar with Boost.mp11. I recommend removing the reference to
mp_all and instead use something similar to what the Standard does:
http://eel.is/c++draft/variant.ctor#11

Specification of function `emplace<I>()`:
It says:

>
> On exception: If the list of alternatives contains monostate, the
> contained value is either unchanged, or monostate{};
>

In what situation on exception is the contained value left unchanged? This
question applies to the next bullet as well.

The specification also doesn't mention that `.emplace<I>()` requires Ti to
be move-constructible in some cases.

It also says:

Throws: Nothing unless the initialization of the new contained value
throws.

Which sounds strange, because it does throw something. In other places you
use "Throws: Any exception thrown by the initialization of the contained
value.", which makes more sense. Also, the Standard uses, "Throws: Any
exception thrown during the initialization of the contained value.
<http://eel.is/c++draft/variant.mod#11.sentence-1>":
http://eel.is/c++draft/variant.mod#11

swap():
What does it throw and when? Other functions specify it but this one does
not.

get<I>():
Specification says,

>
> If v.index() is I, returns a reference to the object stored in the
> variant. Otherwise, throws bad_variant_access.
>

This means that if I provide an index that is far greater than
`sizeof...(T)`, the program should compile fine and throw an exception at
run-time. This is not what the implementation does: I get a compile-time
error, which is good, but inconsistent with the docs. They need an
additional "Requires:" clause.

visit(): I think it is underspecified. It is not immediately clear that the
first parameter is the visitor and the remaining ones are variants.
Parameter names F and V do not convey this clearly. Either use Visitor and
Variants... or provide a longer description, even if informal. The docs
really should contain an example of `visit()` somewhere.

According to the specs, the following should work:

```
void f() {}
variant2::visit(f);
```
which is not incorrect, but strange.

The std::variant also provides a second overload of `visit` that explicitly
defines the return type: http://eel.is/c++draft/variant.visit

This overload is not present in variant2. But this fact is not listed in
the initial section of the docs that lists the differences between the two
libraries.

Regards,
&rzej;