[boost] [variant2] Andrzej's review

Subject: [boost] [variant2] Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-04-13 00:34:35

Hi Everyone,

I recommend that variant2 should be REJECTED in its current shape.

The main (and only) reason for rejection is the never-empty basic
exception-safety guarantee offered and endorsed by the library. I have
serious concerns that by adding it into Boost we would be leveraging the
concept that is bug prone and encouraging bad habits. I have expressed my
concerns in detail in another mail. I find this to be in contrast with what
I perceive as goals of Boost. It is my understanding that the never-empty
basic guarantee is the core of the library therefore the conditional
acceptance would not work. My recommendation would be different if the
library dropped the goal of providing the never-empty guarantee, or if it
provided a strong guarantee in the cases where assignment emplacement
required to change the alternative.

I honestly appreciate the work Peter has done to provide this library, but
I strongly disagree with the primary design decision.

If the library is still accepted I recommend the following things:

1. Call it something slightly different so that at least namespace name
clearly indicates that it is not an "improved std::variant" or "improved
boost::variant" but just another offer in the market with different design
choices in the design space: maybe ne_variant (for "Never Empty") or
si_variant (for "Strong Invariant"), or even pdimov_variant (for "Perfectly
Defined Invariant Model Of Variant").

2. Drop the implementation of std::expected. It does not fit into the same
library even if 90% of the implementaiton is shared. Propose a second
library even if what it does is to #include the variant.

3. Move functions unsafe_get() from namespace detail and make them part of
the official interface. You need to use them yourself in the implementation
of visit() because otherwise you could not implement it efficiently. They
are part of the minimum efficient base interface. Even if required very
occasionally.

4. Provide a 2 minute introduction to the library for the newbies. We do
not need a full tutorial. A link to someone else's description will do, but
a short introduction I think is a must. I offer to help write one.

5. Provide a design rationale explaining why the never-guarantee is offered
at all and why in this way. Why you are disregarding the design choices of
boost::variant's never-empty guarantee. Also provide a detailed description
on which algorithm is chosen under what circumstances and what it does to
provide the guarantee.

Now, to review questions:

- What is your evaluation of the design?

As noted, I strongly disagree with the main design choice of the library:
the never-empty guarantee. I see no positive value, I see negative value.
This aside, I do not see much design choices here: it is a quite small
vocabulary type.

- What is your evaluation of the implementation?

Of all reviews I have participated in I have never found it so easy to
understand the implementation. I find it very clean and short. I appreciate
that it is a single header library. I can just copy the content of the file
and test it in WandBox.

Also the implementation is a very good practical tutorial on mp11. Peter
already wrote good tutorials on mp11, but here for the first time I have
seen it used for something that I consider a real life application. The
implementation of variant looks so simple with mp11.

- What is your evaluation of the documentation?

The reference part meets the high standards of Boost. I am missing even a
short introduction that would give us the motivating example. Some have
said that it is enough to say "find the tutorial on std::variant". I
disagree: there is no good introduction to variant, and I think Boost
libraries have a good history for providing a good problem domain
description apart form the implementation. I also think that given the
controversies around the never-empty guarantee the design choice requires a
detailed information in the docs.

- What is your evaluation of the potential usefulness of the library?

Given that I find std::variant design superior, I suspect this library
would be useful in environments that cannot afford to use C++17 yet. It has
advantages over boost::variant. It has cleaner implementation.
boost::variant is giving me lots of warnings in clang-tidy. I expect these
to be gone in variant2. Also the trade-offs made in double buffering may be
superior to those in boost::variant: no memory allocation, no additional
potential reason for throwing.

- Did you try to use the library? With which compiler(s)? Did you have any
problems?

I tried to break the library with nasty cases in various clang and GCC
versions. I reported some minor bugs over GitHub. Peter is quickly
addressing them. The library does not compile in GCC 8.2 and higher in
C++17 mode due to some bug in GCC. This is already tracked as issue.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I have spent a number of evenings reading the docs, the sources, playing
with the library, debugging, discussing the design in the mailing list. I
also followed the discussion in the C++ Standards Committee, where similar
issues with never-empty guarantee were discussed.

- Are you knowledgeable about the problem domain?

I have a lot of experience with different implementations of `optional`: I
am maintaining the boost version, I have provided the wording and the
design for the std version and provided the reference implementation
thereof (at https://github.com/akrzemi1/Optional/) which turned out
surprisingly popular. Optional needs to solve similar problems in the
implementation: manually managing a raw buffer or a union, and has similar
problems with exception safety guarantees. However it does not need to
address the never-empty guarantee issue. I also use a lot of boost::variant
at work.

Finally, I want to thank Peter for writing this library and submitting it
for review. The review itself was very instructive.

Regards,
&rzej;