I'll firstly apologise for the brevity of this review. It is just before the ACCU conference where I am speaking on a topic I have little knowledge of, and I have done almost no prep due to insufficient hours in the day. I am aware that if I don't make a review now, then I won't at all, so this will have to do. 1. Overall I like the library, it adds significant benefit (primarily removal of dynamic memory allocation) over Boost.Variant. 2. I don't like the lack of propagation of triviality of each individual special member functions. std::variant does this. std::expected does this. I can see Peter's point that for long lists of alternative states, implementing this is a lot of pain for almost no gain. However I do NOT agree with him for Expected. For a two-state variant, it is very highly likely that triviality of special member function will be very important for not a small number of use cases. I personally am happy for Variant2 to implement Expected without review approval here IF that Expected matches exactly std::expected. I am NOT happy if Variant2's Expected breaks conformance on triviality of special member function. If that is the only option on the table, then either please do not include an Expected implementation, or don't include Variant2. This raises a wider point though. What is good for a two-state variant still applies, though less so, to three-state variants, four-state variants and so on. I think there is a fundamental design issue here. If Variant2 can't propagate triviality of individual special member functions for small-number-of-state variants, then there is something fundamentally wrong here. 3. I find the design choice to randomly permute state to avoid an empty state baffling when there is a double buffer based design choice available. I do not like, one bit, the random permutation of state. I think it will lead to buggy code with hard-to-test corner cases. I think it needs to go away in the proposed design. I also don't get the dislike of the double buffer based design. Peter seems to really dislike it, and avoids it whenever possible. I can't understand why. If you can employ a double buffer design in order to eliminate random permutation of state, to me this is an unalloyed good in every way. Nowhere does it say that a variant must be no larger than the largest of its possible states. There is nothing wrong, *whatsoever*, with a variant which is sized the largest of its possible states multiplied by two. So please change the design to that instead. That should be the default design, and design claims. Programmers should write code assuming that design. IF it can be proven that a double buffer is unnecessary e.g. all alternative types are noexcept, a monostate alternative is available etc, THEN you can employ a single buffer implementation as an *optimisation*. That makes thing simple for the end user: add monostate, get single buffer sized. Otherwise assume double buffer sized. With a default double buffer design, most of the other issues which other reviewers have raised go away. Even Robert's concerns about assignment operators, with a double-buffer-by-default design one can always implement the strong guarantee. I badly need to depart, so I'll have to end there by saying that I vote to ACCEPT, conditional on (i) propagation of triviality of special member functions for at least small numbers of alternative states and (ii) removal of ANY POSSIBILITY of random permutation of chosen state under ANY circumstances. Finally, thanks Peter for bringing this library for review. Nice library. Niall
participants (1)
-
Niall Douglas