[boost] Variant2 review

Subject: [boost] Variant2 review
From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2019-04-07 16:03:00

Dear All,

Here is my brief review of Peter Dimov's proposed Boost.Variant2.  Sorry 
that this is not a particularly thorough review.  I have read the docs, 
scanned the code, and followed the discussion on the mailing list; I have 
not tried to compile or use it.

I have recently been using the original Boost.Variant for the first time. 
I actually started writing code based on std::variant, but then realised 
that I don't have support for it on one C++14-only platform, so changed 
to Boost.Variant.  It's unfortunate that Boost.Variant has a somewhat 
different interface from std::variant, so it would be useful to have a 
Boost variant that is (more) compatible with std::variant.  It would also 
be useful if it were somewhat backward compatible with the old Boost.Variant. 
The proposed Boost.Variant2 is compatible with std::variant; could some 
thin layer of compatibility be added to make it possible to port from 
Boost.Variant code?  Specifically, the differences I have noticed are 
apply_visitor vs. visit and get(V*) vs. get_if.

In my case, my stored types are typically raw pointers or small POD structs; 
none of these can throw during construction/assignment so the complicated 
arguments about exception guarantees are not applicable.  Personally for 
my current application I'd be happy with a simpler variant that requires 
noexcept types, or disables assignment for types that might throw, or just 
doesn't offer an exception safety guarantee.  Ideally, perhaps a 
Boost.Variants *library* should provide a selection of different variants 
with different features?

That leads to the question of naming; we have boost::variant, boost::variant2 
and std::variant and those names tell us nothing about how they differ. 
I'm unsure how or if this can be improved; I'm not suggesting that variant2 
should be renamed variant_never_empty_maybe_double_size!  But maybe the 
long-name of the library, i.e. what people will see when they look at the 
list of Boost libraries, could say "Never empty variant" rather than 
"Variant 2"?

Prior to the review I was familiar with the issue of dynamic allocation, 
the fact that is is needed in some cases by some variant implementations, 
and the desire to avoid it.  However I was not aware of the alternative of 
storing two copies of the union, doubling the size of the variant.  
Much of my work is with memory-constrained systems and unknowingly 
using twice as much memory as expected would be very undesirable.  (I 
noticed Niall Douglas' comment earlier today "There is nothing wrong, 
*whatsoever*, with a variant which is sized the largest of its possible 
states multiplied by two" - I strongly disagree with that.)  I'd like 
to at least get a warning if that were going to happen.  A specific 
concern is that I might accidentally get this fallback due to forgetting 
to write "noexcept" somewhere (e.g. when using types from older code).

On the subject of size, I note that the index is stored in an int, while 
virtually all uses would surely need fewer than 256 (or certainly 2^16) 
alternatives.  Of course changing the index to uint16_t would not actually 
help on many platforms due to structure alignment requirements.  This 
makes me wonder about a specialised variant for pointers that uses the 
spare bits in (most/some) 64-bit pointers to store the variant index. 
This could usefully be part of a variants *library*.

I read Andrzej Krzemienski's comments about the documentation and largely 
agree with them - though I would say that this library is close enough to 
std::variant that perhaps the right thing to do is simply to say "go and 
read about std::variant and then come back here to see the differences".

Peter posted a link to some benchmark results last week; those numbers 
are encouraging and it is also good to know that Peter actually made the 
effort to measure them!  I do wonder how a "trivial" variant supporting 
only noexcept types would compare, though.

Boost.Variant has something called polymorphic_get.  I am unclear whether 
Boost.Variant2 (and/or std::variant) somehow support this functionality.

The situation re. expected is unfortunate.  I believe really the review 
should have waited for it to be completed.   Peter's comment that if it 
is decided that it needs to be reviewed then he would remove it and 
never resubmit it was ... surprisingly undiplomatic?  I have always 
found it a bit odd that Boost requires a thorough review of an initial 
library submission but nothing at all for later additions.

In summary:

- I believe the library should be accepted.

- It would be great if some compatibility features with Boost.Variant could 
be added, to make it possible for users of that older library to transition 
more easily.  That is not a condition for acceptance.

- I would love to have some way to get a warning or error if the double 
storage mode had been triggered, or to disable that mode (with an error).

- It would be great to see some alternative variants, with different 
features / trade-offs, included in the library.  Ideally, naming of types 
and headers should allow for that.  I'm not suggesting that Peter should 
be personally expected to supply these alternatives, but perhaps he 
would accept offerings from others.

- I think we should allow Peter to "slip in" his expected when it is 
ready; I say this largely because Peter has a long history with Boost. 
Future review managers should try to avoid situations like this arising.

Regards, Phil.