[boost] [variant2] Gavin's review

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 seems both ugly and unnecessary. Also it should probably be included after . 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.