Boost logo

Boost :

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.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk