Hi Everyone, I recommend that variant2 should be REJECTED in its current shape. The main (and only) reason for rejection is the never-empty basic exception-safety guarantee offered and endorsed by the library. I have serious concerns that by adding it into Boost we would be leveraging the concept that is bug prone and encouraging bad habits. I have expressed my concerns in detail in another mail. I find this to be in contrast with what I perceive as goals of Boost. It is my understanding that the never-empty basic guarantee is the core of the library therefore the conditional acceptance would not work. My recommendation would be different if the library dropped the goal of providing the never-empty guarantee, or if it provided a strong guarantee in the cases where assignment emplacement required to change the alternative. I honestly appreciate the work Peter has done to provide this library, but I strongly disagree with the primary design decision. If the library is still accepted I recommend the following things: 1. Call it something slightly different so that at least namespace name clearly indicates that it is not an "improved std::variant" or "improved boost::variant" but just another offer in the market with different design choices in the design space: maybe ne_variant (for "Never Empty") or si_variant (for "Strong Invariant"), or even pdimov_variant (for "Perfectly Defined Invariant Model Of Variant"). 2. Drop the implementation of std::expected. It does not fit into the same library even if 90% of the implementaiton is shared. Propose a second library even if what it does is to #include the variant. 3. Move functions unsafe_get() from namespace detail and make them part of the official interface. You need to use them yourself in the implementation of visit() because otherwise you could not implement it efficiently. They are part of the minimum efficient base interface. Even if required very occasionally. 4. Provide a 2 minute introduction to the library for the newbies. We do not need a full tutorial. A link to someone else's description will do, but a short introduction I think is a must. I offer to help write one. 5. Provide a design rationale explaining why the never-guarantee is offered at all and why in this way. Why you are disregarding the design choices of boost::variant's never-empty guarantee. Also provide a detailed description on which algorithm is chosen under what circumstances and what it does to provide the guarantee. Now, to review questions: - What is your evaluation of the design? As noted, I strongly disagree with the main design choice of the library: the never-empty guarantee. I see no positive value, I see negative value. This aside, I do not see much design choices here: it is a quite small vocabulary type. - What is your evaluation of the implementation? Of all reviews I have participated in I have never found it so easy to understand the implementation. I find it very clean and short. I appreciate that it is a single header library. I can just copy the content of the file and test it in WandBox. Also the implementation is a very good practical tutorial on mp11. Peter already wrote good tutorials on mp11, but here for the first time I have seen it used for something that I consider a real life application. The implementation of variant looks so simple with mp11. - What is your evaluation of the documentation? The reference part meets the high standards of Boost. I am missing even a short introduction that would give us the motivating example. Some have said that it is enough to say "find the tutorial on std::variant". I disagree: there is no good introduction to variant, and I think Boost libraries have a good history for providing a good problem domain description apart form the implementation. I also think that given the controversies around the never-empty guarantee the design choice requires a detailed information in the docs. - What is your evaluation of the potential usefulness of the library? Given that I find std::variant design superior, I suspect this library would be useful in environments that cannot afford to use C++17 yet. It has advantages over boost::variant. It has cleaner implementation. boost::variant is giving me lots of warnings in clang-tidy. I expect these to be gone in variant2. Also the trade-offs made in double buffering may be superior to those in boost::variant: no memory allocation, no additional potential reason for throwing. - Did you try to use the library? With which compiler(s)? Did you have any problems? I tried to break the library with nasty cases in various clang and GCC versions. I reported some minor bugs over GitHub. Peter is quickly addressing them. The library does not compile in GCC 8.2 and higher in C++17 mode due to some bug in GCC. This is already tracked as issue. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I have spent a number of evenings reading the docs, the sources, playing with the library, debugging, discussing the design in the mailing list. I also followed the discussion in the C++ Standards Committee, where similar issues with never-empty guarantee were discussed. - Are you knowledgeable about the problem domain? I have a lot of experience with different implementations of `optional`: I am maintaining the boost version, I have provided the wording and the design for the std version and provided the reference implementation thereof (at https://github.com/akrzemi1/Optional/) which turned out surprisingly popular. Optional needs to solve similar problems in the implementation: manually managing a raw buffer or a union, and has similar problems with exception safety guarantees. However it does not need to address the never-empty guarantee issue. I also use a lot of boost::variant at work. Finally, I want to thank Peter for writing this library and submitting it for review. The review itself was very instructive. Regards, &rzej;
On Fri, Apr 12, 2019 at 5:35 PM Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
The main (and only) reason for rejection is the never-empty basic exception-safety guarantee offered and endorsed by the library.
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it? Thanks
On Sat, 13 Apr 2019 at 00:47, Vinnie Falco via Boost <boost@lists.boost.org> wrote:
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it?
Andrzej argues (in the other post) that you *shouldn't want* it, becoz it masks UB [result of programmer error] and will now cause havoc elsewhere in the program. Additionally he says he does not want to 'pay' for moving the bug elsewhere, as it's not worth paying for, and potentially [more] dangerous. degski -- *Microsoft, please kill Paint3D*
On Fri, Apr 12, 2019 at 9:40 PM degski via Boost <boost@lists.boost.org> wrote:
On Sat, 13 Apr 2019 at 00:47, Vinnie Falco via Boost <
boost@lists.boost.org>
wrote:
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it?
Andrzej argues (in the other post) that you *shouldn't want* it, becoz it masks UB [result of programmer error] and will now cause havoc elsewhere in the program.
The basic guarantee prevents UB, doesn't mask it. Perhaps you mean to say "masks bugs", or at least that's how I read Andrzej's opinion. This may be so, but I fail to see why the argument against the basic guarantee in variant2 assignment does not apply in general. It would help me (and perhaps others) understand this argument if someone explains why is the basic guarantee appropriate in, say, the std::vector<T>::op=, but not in variant.
On Sat, 13 Apr 2019 at 05:16, Emil Dotchevski via Boost < boost@lists.boost.org> wrote:
On Fri, Apr 12, 2019 at 9:40 PM degski via Boost <boost@lists.boost.org> wrote:
On Sat, 13 Apr 2019 at 00:47, Vinnie Falco via Boost <
boost@lists.boost.org>
wrote:
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it?
Andrzej argues (in the other post) that you *shouldn't want* it, becoz it masks UB [result of programmer error] and will now cause havoc elsewhere in the program.
The basic guarantee prevents UB, doesn't mask it.
Perhaps you mean to say "masks bugs", or at least that's how I read Andrzej's opinion.
Yes, 'masks bugs', standing corrected. This may be so, but I fail to see why the argument
against the basic guarantee in variant2 assignment does not apply in general. It would help me (and perhaps others) understand this argument if someone explains why is the basic guarantee appropriate in, say, the std::vector<T>::op=, but not in variant.
My answer is [not that you asked for it], I don't know. I can absolutely see the value of a strong guarantee, but not of the basic guarantee. degski -- *Microsoft, please kill Paint3D*
sob., 13 kwi 2019 o 07:16 Emil Dotchevski via Boost <boost@lists.boost.org> napisał(a):
On Fri, Apr 12, 2019 at 9:40 PM degski via Boost <boost@lists.boost.org> wrote:
On Sat, 13 Apr 2019 at 00:47, Vinnie Falco via Boost <
boost@lists.boost.org>
wrote:
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it?
Andrzej argues (in the other post) that you *shouldn't want* it, becoz it masks UB [result of programmer error] and will now cause havoc elsewhere in the program.
The basic guarantee prevents UB, doesn't mask it.
Perhaps you mean to say "masks bugs", or at least that's how I read Andrzej's opinion. This may be so, but I fail to see why the argument against the basic guarantee in variant2 assignment does not apply in general. It would help me (and perhaps others) understand this argument if someone explains why is the basic guarantee appropriate in, say, the std::vector<T>::op=, but not in variant.
Let me clarify. I am not arguing against the basic guarantee. (I am all in favor of basic guarantee.) When I mention a hypothetical variant that enters a valueless_by_exception state, and later it is UB if its value is read, I still consider it a basic guarantee. It is just that such type has a weaker invariant than variant2. It is still considered basic, because you can perform the minimum set of operations: destroy it and reset it. My point is: - The weaker invariant *in the case of variant* does not impose the additional burden on programmers and does not require of them to put defensive if-statements before every use of the variant. (Therefore maybe it is better if this is described by two invariants: one for normal operations, and one for special-case situations.) - Basic never empty guarantee is no better than any other basic guarantee. I never argue that std::vector's invariant should be weaker than what it is now, because the invariant being stronger than necessary does not cost us any run-time or spatial performance. It is trivial to provide, does not increase the size of the vector, it does not require the vector to make additional allocations, it does not complicate the implementation a single bit. In fact, it would require efficiency compromises to provide another "partially defined" state. However, I still believe that anyone who observes the value of a vector that threw from its assignment has done their exception handling logic wrong. And maybe it would be beneficial if in debug mode (or some other special testing mode) vector contained a flag that it has thrown from any of its basic-guarantee operations; and an attempt to observe its value should be signaled with assertion. Regards, &rzej;
On Fri, Apr 12, 2019 at 7:47 PM Vinnie Falco via Boost < boost@lists.boost.org> wrote:
On Fri, Apr 12, 2019 at 5:35 PM Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
The main (and only) reason for rejection is the never-empty basic exception-safety guarantee offered and endorsed by the library.
If I want/need a variant that offers the never-empty basic guarantee then Boost should not provide it?
Maybe? How many variants should Boost provide (and I'm not asking that facetiously)? These tradeoffs are not the ones I would make. There are two other variants I would rather use in different circumstances: - Strong exception safety - Strong performance (never fall back on noexcept move or double buffering to avoid a possible exception) I don't know how to vote for this. The design tradeoffs Peter is making are certainly reasonable ones, but they don't cover the things I do with variant. I would just fall back on using std::variant over the tradeoffs being made here. -- Nevin ":-)" Liber <mailto:nevin@cplusplusguy.com <nevin@eviloverlord.com>> +1-847-691-1404
On 13/04/2019 01:34, Andrzej Krzemienski via Boost wrote:
I recommend that variant2 should be REJECTED in its current shape.
I find this recommendation excessively harsh personally. I agree that randomly permuting state in order to "avoid" an empty state is worse than an empty state. I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising. I do not find an Expected implementation problematic IF triviality of member special function is propagated. It needs to match, exactly, the std::expected edition. In general, I think me and you are thus in general agreement given everything you have discussed and the questions you have raised. I think therefore an acceptance with required conditions of change is the more appropriate recommendation. Reject, in my opinion, is too harsh. Reject is for libraries with no hope of salvation in their present form. Variant2 in my mind is not that. Niall
Niall Douglas wrote:
I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising.
First off, the basic guarantee isn't worse than useless, it's the minimum standard that every non-broken component must meet, and everyone who argues otherwise isn't worth listening to. With that out of the way, though, variant2 may end up with the strong guarantee anyway, for other reasons. Consider this example: variant<X, std::vector<X>> v; v.emplace<1>( 1 ); v = get<1>( v )[ 0 ]; (a variation of which Antony Polukhin reported some time ago as a potential defect in std::variant, if I'm not mistaken.) What happens here is that on the last line, `v` on the left owns the `X` value on the right. So when the implementation first destroys the old contents of `v` to make room for the new `X`, the right hand side is destroyed, and then undefined behavior occurs when we try to copy it into `v`. This is fixable. But when we fix it, we've paid much of the price required for the strong guarantee, so we might as well offer that. With respect to this example, there're the usual two schools of thought, one saying it's the user's fault, don't penalize my variant for that, the other preferring variant handle it. I'm trending towards the latter camp.
On 13/04/2019 16:33, Peter Dimov via Boost wrote:
Niall Douglas wrote:
I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising.
First off, the basic guarantee isn't worse than useless, it's the minimum standard that every non-broken component must meet, and everyone who argues otherwise isn't worth listening to.
Sorry, that's the wrong bunching of my words (my fault). What I specifically meant was "worse-than-useless choice of implementation by variant2 of the basic guarantee which is only technically valid, but is certainly surprising". So, to be clear here, my issue is with *your* choice of how to meet the basic guarantee. I think you're ticking the box technically, breaking the guarantee in spirit.
What happens here is that on the last line, `v` on the left owns the `X` value on the right. So when the implementation first destroys the old contents of `v` to make room for the new `X`, the right hand side is destroyed, and then undefined behavior occurs when we try to copy it into `v`.
I'd re-recommend my original advice that you ship a double buffer variant, and a single buffer variant, and let the user choose when they want to use which. The documentation ought to recommend to the double buffer variant as the correct default choice. And document the many tradeoffs, including far-too-easy-to-do-UB, randomly permuting state (another source of bugs), etc in the single buffer variant. In other words, single buffer variant is a power users choice, not to be chosen without caution. Niall
On Sat, Apr 13, 2019 at 8:57 AM Niall Douglas via Boost < boost@lists.boost.org> wrote:
On 13/04/2019 16:33, Peter Dimov via Boost wrote:
Niall Douglas wrote:
I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising.
First off, the basic guarantee isn't worse than useless, it's the minimum standard that every non-broken component must meet, and everyone who argues otherwise isn't worth listening to.
Sorry, that's the wrong bunching of my words (my fault).
What I specifically meant was "worse-than-useless choice of implementation by variant2 of the basic guarantee which is only technically valid, but is certainly surprising".
I'll repeat myself, I fail to see any logic in this statement that doesn't apply to std::vector::op=. Yes, with variant2 the contained type may change, whereas the type of the objects in the vector won't change, but this is irrelevant. It's like saying well, at least variant won't change its size, whereas a vector could, and that is "technically valid but is certainly surprising". Yes indeed, when an error occurs, you might find objects in a state that you didn't set explicitly. Being surprised that the state may change is equivalent to not understanding how error handling works under the basic guarantee.
What I specifically meant was "worse-than-useless choice of implementation by variant2 of the basic guarantee which is only technically valid, but is certainly surprising".
I'll repeat myself, I fail to see any logic in this statement that doesn't apply to std::vector::op=.
Yes, with variant2 the contained type may change, whereas the type of the objects in the vector won't change, but this is irrelevant. It's like saying well, at least variant won't change its size, whereas a vector could, and that is "technically valid but is certainly surprising".
Yes indeed, when an error occurs, you might find objects in a state that you didn't set explicitly. Being surprised that the state may change is equivalent to not understanding how error handling works under the basic guarantee.
The problem is the hard coding of random change of state on exception throw during assignment. Yes, a vector may be in some weird state after an exception throw middle of operator=. But that's on you for your choice of T in vector<T>. It's not hard coded into vector's design as a default, so it's imposed on all users of vector. Niall
On Sat, Apr 13, 2019 at 3:14 PM Niall Douglas via Boost < boost@lists.boost.org> wrote:
What I specifically meant was "worse-than-useless choice of implementation by variant2 of the basic guarantee which is only technically valid, but is certainly surprising".
I'll repeat myself, I fail to see any logic in this statement that
apply to std::vector::op=.
Yes, with variant2 the contained type may change, whereas the type of
doesn't the
objects in the vector won't change, but this is irrelevant. It's like saying well, at least variant won't change its size, whereas a vector could, and that is "technically valid but is certainly surprising".
Yes indeed, when an error occurs, you might find objects in a state that you didn't set explicitly. Being surprised that the state may change is equivalent to not understanding how error handling works under the basic guarantee.
The problem is the hard coding of random change of state on exception throw during assignment. Yes, a vector may be in some weird state after an exception throw middle of operator=.
The state is not weird, in fact it is guaranteed to be valid.
But that's on you for your choice of T in vector<T>. It's not hard coded into vector's design as a default, so it's imposed on all users of vector.
Yes it is very much in the vector design. Someone correct me if I'm wrong, but my understanding is that a conforming implementation of vector<int>::op= may first free the currently allocated memory, then fail to allocate. The state of the object will change, and this has nothing to do with int, everything to do with the basic guarantee. In addition, could you clarify how is the choice of T in vector<T> different than the choice of T and U in variant<T, U>? After all, variant2::variant<int, char>::op= can't fail.
On Sat, 13 Apr 2019 at 15:34, Peter Dimov via Boost <boost@lists.boost.org> wrote:
... and everyone who argues otherwise isn't worth listening to.
What happened to writing code that does the right thing in the first place [the kind of code that doesn't depend on any guarantee], like most C-code [the kind of code one could write in C++ as well, the kind of code that runs OS's]? Like I said earlier, strong guarantee yes [keep my database intact, please], anything else, duh. degski -- *Microsoft, please kill Paint3D*
On 14/04/2019 22:16, degski via Boost wrote:
On Sat, 13 Apr 2019 at 15:34, Peter Dimov via Boost <boost@lists.boost.org> wrote:
... and everyone who argues otherwise isn't worth listening to.
What happened to writing code that does the right thing in the first place [the kind of code that doesn't depend on any guarantee], like most C-code [the kind of code one could write in C++ as well, the kind of code that runs OS's]? Like I said earlier, strong guarantee yes [keep my database intact, please], anything else, duh.
degski
when has this code existed, in some mythical land no doubt, code without bugs is a practical and theoretical impossibility proved years ago -- .~. In my life God comes first.... /V\ but Linux is pretty high after that :-D /( )\ Francis (Grizzly) Smit ^^-^^ http://www.smit.id.au/
On Sun, 14 Apr 2019 at 12:56, Francis Grizzly Smit via Boost < boost@lists.boost.org> wrote:
when has this code existed, in some mythical land no doubt, code without bugs is a practical and theoretical impossibility proved years ago
So, it will need fixing, it should not depend on some library artifact that stops a program from crashing (unless it's my database and I want the strong guarantee), but otherwise doesn't do anything useful. I think printing a message "The programmer messed it up" suffices. degski -- *Microsoft, please kill Paint3D*
On 13.04.19 17:33, Peter Dimov via Boost wrote:
First off, the basic guarantee isn't worse than useless, it's the minimum standard that every non-broken component must meet, and everyone who argues otherwise isn't worth listening to.
I would say that the absolute minimum requirement is that all objects are left destructible. It is not a requirement that all possible operations on an object remain defined behavior - and there is plenty of precedent for certain operations not being allowed on certain objects even in the absence of exceptions. A variant without the never-empty guarantee can meet that requirement, even if the empty state is an exceptional state that can only be entered through an exception. There may be valid arguments for a never-empty variant, but it is possible to write correct code that recovers from exceptions even with an exceptional-empty variant. I'm not arguing against the basic exception guarantee per se. I am arguing for an interpretation of the basic guarantee that allows for exceptional states. -- Rainer Deyke (rainerd@eldwood.com)
sob., 13 kwi 2019 o 17:34 Peter Dimov via Boost <boost@lists.boost.org> napisał(a):
Niall Douglas wrote:
I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising.
First off, the basic guarantee isn't worse than useless, it's the minimum standard that every non-broken component must meet, and everyone who argues otherwise isn't worth listening to.
With that out of the way, though, variant2 may end up with the strong guarantee anyway, for other reasons. Consider this example:
variant<X, std::vector<X>> v;
v.emplace<1>( 1 );
v = get<1>( v )[ 0 ];
(a variation of which Antony Polukhin reported some time ago as a potential defect in std::variant, if I'm not mistaken.)
What happens here is that on the last line, `v` on the left owns the `X` value on the right. So when the implementation first destroys the old contents of `v` to make room for the new `X`, the right hand side is destroyed, and then undefined behavior occurs when we try to copy it into `v`.
This is fixable. But when we fix it, we've paid much of the price required for the strong guarantee, so we might as well offer that.
With respect to this example, there're the usual two schools of thought, one saying it's the user's fault, don't penalize my variant for that, the other preferring variant handle it. I'm trending towards the latter camp.
Whatever the decision, it is worth keeping this characterization in mind: this is two schools of thought. It should never disappear from the library documentation: in which favor do we resolve the trade-offs. Regards, &rzej;
sob., 13 kwi 2019 o 12:45 Niall Douglas via Boost <boost@lists.boost.org> napisał(a):
On 13/04/2019 01:34, Andrzej Krzemienski via Boost wrote:
I recommend that variant2 should be REJECTED in its current shape.
I find this recommendation excessively harsh personally.
I agree that randomly permuting state in order to "avoid" an empty state is worse than an empty state.
I just do not understand the antipathy here to a double-buffered-by-default design, and thus the strong guarantee can be easily made, rather than a worse-than-useless basic guarantee which is only technically valid, but is certainly surprising.
I do not find an Expected implementation problematic IF triviality of member special function is propagated. It needs to match, exactly, the std::expected edition.
In general, I think me and you are thus in general agreement given everything you have discussed and the questions you have raised. I think therefore an acceptance with required conditions of change is the more appropriate recommendation. Reject, in my opinion, is too harsh. Reject is for libraries with no hope of salvation in their present form. Variant2 in my mind is not that.
Thanks for raising this. Indeed, we seem to agree on the desired design of a variant that is supposed to be an alternative to std::variant. A strong guarantee, or a "semi-strong" guarantee (If T's assignment isn't strong, we can just call it). I personally would probably never use it: I never need this guarantee, but it is consistent, easy to understand, gotcha-free, does not encourage buggy code, and is sufficiently different from std::variant to warrant its existence. We do disagree on the procedural things, though. So let me try to explain. Regarding the implementation of Expected. I didn't even look at it. I barely had time to review the Variant. (Thank you, Michael, for extending the review period.) I trust that Peter's implementation is good. My objection is not to putting an implementation of std::expected into Boost, but to hiding it in a library called "Variant". I would rather encourage Peter to provide it as a dedicated Library, even if this other library says "it is a variant inside". Even if Expected uses Variant2 inside today it may change in the future, if additional constraints are imposed on std::expected, which make no sense for Variant2. Also, we seem to ave different interpretation of what "Reject" means. I am following the distinction that I observed in the first days of Boost: Conditionally accept -- Any reviewing is finished. Get the library in and just fix this and that, subject to a mini-review, which is just a control of whether the conditions have been met Reject and encourage a re-submission -- This is quite similar, except that we have another formal review. I think we need another review. Not that we do not trust Peter: far from it. But given the strong controversy around the exception safety guarantee for assignment/emplacement, we -- the community -- need the review to discuss and understand the problem and design space better. To get some sort of consensus that we can use for promoting and teaching the boost::variant in a uniform way. I hope Peter is not discouraged by recommendation. Regards, &rzej;
participants (10)
-
Andrzej Krzemienski -
degski -
Emil Dotchevski -
Francis Grizzly Smit -
Nevin Liber -
Niall Douglas -
Peter Dimov -
Rainer Deyke -
Robert Ramey -
Vinnie Falco