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
participants (1)
-
Gavin Lambert