[boost] [variant2] Gavin's review

Subject: [boost] [variant2] Gavin's review
From: Gavin Lambert (boost_at_[hidden])
Date: 2019-04-12 00:29:21

This is my review of Variant2.

I vote to ACCEPT this library.

However I find the ambiguous state of "expected" a bit too ambiguous 
(and the "expected" concept in general has caused quite a bit of 
discussion and contention in the past) so I would prefer that it not be 
considered part of the library as is, and go through at least a 
mini-review before entering Boost.  I'm not suggesting that this be a 
formal acceptance condition, however.  (I did not look at this type for 
my review.)

I also question whether a more descriptive name than "Variant2" can be 
used for it, although I recognise that at least starting the library 
name with "variant" is a good idea to aid discoverability in alphabetic 
sorting order.

   - What is your evaluation of the design?

As I've stated elsewhere, I feel that if you're willing to pay the cost 
of double-buffering then you might as well go all-in and implement the 
strong guarantee as well (or at least a partially strong guarantee, 
where any attempt to change the stored type provides the full strong 
guarantee but attempts to assign the same type merely delegates to the 
underlying type's assignment operation and so provides the same 
guarantee as that -- this seems sufficiently reasonable and useful to me 
without sacrificing too much performance, and still lets you 
single-buffer as an optimisation when all types are noexcept assignable).

Having said that, I recognise that others have different lines in the 
sand, and this is a reasonable design choice within those lines.

   - What is your evaluation of the implementation?

Overall, good.  I do have a few minor nitpicks:

1. bad_variant_access's constructor should be "constexpr = default" to 
preserve constexpr and/or triviality (if std::exception is also 
constexpr and/or trivially constructible, although this is not the case 
in VS2017, but could be in other implementations).

2. When std::monostate exists, it should treat both equivalently, for 
compatibility.  (It should not alias its own to std::monostate, as that 
poses ABI problems if not all code is compiled in C++17 mode.)

3. The duplicated include guard around <boost/mp11.hpp> seems both ugly 
and unnecessary.  Also it should probably be included after 
<boost/config.hpp>.

4. It's not clear why some of the internal methods (such as _get_impl) 
have leading underscores while others (such as emplace_impl) do not.  It 
seems inconsistent.

5. It's not clear what purpose the "none" storage type serves (other 
than apparently requiring the indexing to be offset by one).  It seems 
rather like an empty state... (and the continual offsetting seems 
inefficient; most of it happens at compile time, but not all).

6. variant_base_impl::_replace has a "requires" comment; why doesn't it 
have a matching assert?  Also, why are these not BOOST_ASSERTs?

7. As discussed elsewhere, a public trait should be provided which lets 
you static_assert which implementation was actually instantiated.

   - What is your evaluation of the documentation?

It is minimal but sufficient for readers already familiar with 
std::variant.  (And who can read standardese.)

It is not sufficient for readers not familiar with std::variant.

I think this is mostly fine, but I would prefer the addition of a 
Motivation and/or Design Rationale section which explains why this is 
better than std::variant and boost::variant.

In future it would also be nice to have a Porting section which explains 
what code changes are required to move from those to this.

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

It seems potentially useful for others, although I would personally have 
preferred different design choices.

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

I did not try it.

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

I read through the documentation and code in moderate detail, but not 
in-depth.

   - Are you knowledgeable about the problem domain?

I am familiar with variants, but have not as yet used them much in 
production code.