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

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 always stores either X or Y. "Type-safe discriminated union (variant) type," sounds arcane to novices. Notation variant 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 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;