Hi Everyone, This is not my final review (yet), but I wanted to raise some issues with Regular ops: comparisons and swap. 1. Comparisons. Equality comparison for `result` makes a strange choice for `T`s without comparisons: two such `result<T>`s just compare unequal. This is somewhat contradictory to semantics of Regular types. For value-semantic types we expect that a copy of the original object `x` compares equal to `x`: ``` int i = 1; int j = i; assert (i == j); ``` For some types, copy-construction makes a copy with the same value, but there is no way to define operator==. In this case, in order not to confuse anyone operator== is simply not provided: ``` using transformation = std::function<int(int)>; transformation f = [](int i) { return i; }; transformation g = f; // assert (f == g); <-- does not compile ``` And now, when we wrap it into `result`, we get: ``` // continuing previous example: out::result<transformation> of = f, og = g; f == g; // <-- compiles fine, returns false. ``` Now you are just telling lies. Two objects have the same value, yet they compare unequal! Here is a live example: https://wandbox.org/permlink/I hqgXDfHW6WqAXHW My recommendation would be not to provide comparison operators if either `T` or `EC` does not provide them. 2. Swap For non-trivially-copyable types, it looks like (similarly to std::variant) `result` cannot provide the strong exception safety guarantee. You may need to both move-construct `T` and `EC` and both can fail. ``` void swap(result &o) noexcept(detail::is_nothrow_swappable<value_type>::value // &&detail::is_nothrow_ swappable<error_type>::value) { using std::swap; #ifndef BOOST_NO_EXCEPTIONS this->_state.swap(o._state); try { swap(this->_error, o._error); } catch(...) { swap(this->_state, o._state); throw; } #else swap(this->_state, o._state); swap(this->_error, o._error); #endif } ``` The trick with catch-reswap-rethrow assumes that `T` is more likely to throw while swapping/moving than `EC`, but it might be quite the opposite. Also, it is possible that while reswapping, another exception will be thrown. In general, you cannot guarantee the roll-back, so maybe it would be cleaner for everyone if you just declared that upon throw from swap, one cannot rely on the state of `result`: it should be reset or destroyed. Another problem is that in the noexcept() clause you are mentioning is_nothrow_swappable<value_type> and is_nothrow_swappable<error_type>, but in the implementation you might be using move constructors rather than swaps. This means that for types that do not throw in swap but throw in move constructor (like std::list), a throw from move constructor will cause a call to `std::terminate`. See live example here: https://wandbox.org/permlink/8jIyb0fx7U72cZSt The noexcept clause should be extended to include move-constructors. This is the case for `std::optional`. Regards, &rzej;
2018-01-22 14:59 GMT+01:00 Andrzej Krzemienski <akrzemi1@gmail.com>:
2. Swap
For non-trivially-copyable types, it looks like (similarly to std::variant) `result` cannot provide the strong exception safety guarantee. You may need to both move-construct `T` and `EC` and both can fail.
Plus, documentation does not mention that the swap provides only a basic guarantee. Regards, &rzej;
My recommendation would be not to provide comparison operators if either `T` or `EC` does not provide them.
Agreed. Logged to https://github.com/ned14/outcome/issues/107
The trick with catch-reswap-rethrow assumes that `T` is more likely to throw while swapping/moving than `EC`, but it might be quite the opposite. Also, it is possible that while reswapping, another exception will be thrown. In general, you cannot guarantee the roll-back, so maybe it would be cleaner for everyone if you just declared that upon throw from swap, one cannot rely on the state of `result`: it should be reset or destroyed.
So, don't bother attempting to restore a valid state at all? Does everybody else agree?
Another problem is that in the noexcept() clause you are mentioning is_nothrow_swappable<value_type> and is_nothrow_swappable<error_type>, but in the implementation you might be using move constructors rather than swaps. This means that for types that do not throw in swap but throw in move constructor (like std::list), a throw from move constructor will cause a call to `std::terminate`. See live example here: https://wandbox.org/permlink/8jIyb0fx7U72cZSt
Surely if std::is_nothrow_swappable tells lies, it is only right that that equals std::terminate? In other words, if the STL traits tell me that `swap(A, B)` does not throw, and then it throws because it's implemented with move constructors, that's surely not my problem?
Plus, documentation does not mention that the swap provides only a basic guarantee.
Agreed. Logged to https://github.com/ned14/outcome/issues/108. Thanks for the feedback! Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2018-01-22 15:53 GMT+01:00 Niall Douglas via Boost <boost@lists.boost.org>:
My recommendation would be not to provide comparison operators if either `T` or `EC` does not provide them.
Agreed. Logged to https://github.com/ned14/outcome/issues/107
The trick with catch-reswap-rethrow assumes that `T` is more likely to throw while swapping/moving than `EC`, but it might be quite the opposite. Also, it is possible that while reswapping, another exception will be thrown. In general, you cannot guarantee the roll-back, so maybe it would be cleaner for everyone if you just declared that upon throw from swap, one cannot rely on the state of `result`: it should be reset or destroyed.
So, don't bother attempting to restore a valid state at all?
To me, restoring makes sense if you can guarantee that it succeeds. Maybe if you do it only if `EC` is nothrow-swappable.
Does everybody else agree?
Another problem is that in the noexcept() clause you are mentioning is_nothrow_swappable<value_type> and is_nothrow_swappable<error_type>, but in the implementation you might be using move constructors rather than swaps. This means that for types that do not throw in swap but throw in move constructor (like std::list), a throw from move constructor will cause a call to `std::terminate`. See live example here: https://wandbox.org/permlink/8jIyb0fx7U72cZSt
Surely if std::is_nothrow_swappable tells lies, it is only right that that equals std::terminate? In other words, if the STL traits tell me that `swap(A, B)` does not throw, and then it throws because it's implemented with move constructors, that's surely not my problem?
No, no. This happens not inside STL, but inside `result`: https://github.com/ned14/boost-outcome/blob/master/include/boost/outcome/det... Inside the implementation of function `value_storage_nontrivial::swap` you are using move construction in certain cases. And this move constructor of `T` can potentially throw and may be declared `noexcept(false)`, but you will not see it if you are only querying for noexcept swap on `T`. Regards, &rzej;
No, no. This happens not inside STL, but inside `result`: https://github.com/ned14/boost-outcome/blob/master/include/boost/outcome/det... Inside the implementation of function `value_storage_nontrivial::swap` you are using move construction in certain cases. And this move constructor of `T` can potentially throw and may be declared `noexcept(false)`, but you will not see it if you are only querying for noexcept swap on `T`.
Oh I see what you mean now. The over tight noexcept spec is a clear bug. Logged to https://github.com/ned14/outcome/issues/109 Thanks for the report. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 23/01/2018 03:53, Niall Douglas wrote:
My recommendation would be not to provide comparison operators if either `T` or `EC` does not provide them.
Agreed. Logged to https://github.com/ned14/outcome/issues/107
I assume this would be individually for each case? There is still benefit in providing operator==(EC) even if T does not provide operator==, eg. for the construct: if (outcome == make_error_code(xxx)) (not sure if this is the exact spelling, but it's something like this) Especially in generic code templated on T. The alternative is much more wordy and not really any clearer. There are also some benefits in retaining the current 'return false' behavior for error comparisons (only), again for generic code that wants to test against several possible heterogeneous specific errors. On the other hand, it probably is safest to make this a compile error to catch the case when someone changes the EC of a method and require the callers to update their checks.
The trick with catch-reswap-rethrow assumes that `T` is more likely to throw while swapping/moving than `EC`, but it might be quite the opposite. Also, it is possible that while reswapping, another exception will be thrown. In general, you cannot guarantee the roll-back, so maybe it would be cleaner for everyone if you just declared that upon throw from swap, one cannot rely on the state of `result`: it should be reset or destroyed.
So, don't bother attempting to restore a valid state at all?
Does everybody else agree?
While it's true that EC could throw on swap, it's probably not very likely given that EC is most likely to be either error_code (or moral equivalent) or a simple enum/int. So that should be reasonably safe in the majority of cases. Swapping back noexcept(false) T might be more problematic; successful swap out is not a guarantee of successful swap back.
2018-01-22 23:31 GMT+01:00 Gavin Lambert via Boost <boost@lists.boost.org>:
On 23/01/2018 03:53, Niall Douglas wrote:
My recommendation would be not to provide comparison operators if either
`T` or `EC` does not provide them.
Agreed. Logged to https://github.com/ned14/outcome/issues/107
I assume this would be individually for each case?
There is still benefit in providing operator==(EC) even if T does not provide operator==, eg. for the construct:
if (outcome == make_error_code(xxx))
(not sure if this is the exact spelling, but it's something like this)
Especially in generic code templated on T. The alternative is much more wordy and not really any clearer.
This could be handled by providing a dedicated heterogeneous comparison between `result<T, EC>` and `EC`. But my preference would be a dedicated function. Regards, &rzej;
On 23/01/2018 12:50, Peter Dimov wrote:
Andrzej Krzemienski wrote:
This could be handled by providing a dedicated heterogeneous comparison between `result<T, EC>` and `EC`.
I prefer `if( !r && r.error() == condition )`. Similarly, `r? *r: x` instead of `r.value_or(x)`.
This is actually a good example of why operator bool might be a bad idea. It's not too controversial for result<>, but in outcome<> "!r" will be true if there isn't a value, but "r.error()" might throw or UB in that case if it has an exception without an error code. So the long-form of that test would be: if ( r.has_error() && r.error() == condition )
It's not too controversial for result<>, but in outcome<> "!r" will be true if there isn't a value, but "r.error()" might throw or UB in that case if it has an exception without an error code.
So the long-form of that test would be:
if ( r.has_error() && r.error() == condition )
For `outcome` yes, for `result` you are guaranteed this is safe: if(!r && r.error() == condition) And just to remind people, `outcome` has a synthesising `failure()` function which does what `exception()` did in Outcome v1, so this is also guaranteed safe: if(!o) std::rethrow_exception(o.failure()) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 23/01/2018 12:35, Andrzej Krzemienski wrote:
2018-01-22 23:31 GMT+01:00 Gavin Lambert:
There is still benefit in providing operator==(EC) even if T does not provide operator==, eg. for the construct: [...] This could be handled by providing a dedicated heterogeneous comparison between `result<T, EC>` and `EC`. But my preference would be a dedicated function.
That already exists; my point was that the argument to conditionally remove operator== might apply to operator==(outcome<>) or operator==(T) but is not necessarily applicable to other operator== cases.
This could be handled by providing a dedicated heterogeneous comparison between `result<T, EC>` and `EC`. But my preference would be a dedicated function.
I'm still not sure if the current "loose comparison" functionality ought to be in Outcome at all. But if it were, it would almost certainly be free functions in the `utils` namespace in my opinion e.g. `utils::loose_compare(a, b)` or something like that. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2018-01-22 23:31 GMT+01:00 Gavin Lambert via Boost <boost@lists.boost.org>:
The trick with catch-reswap-rethrow assumes that `T` is more likely to
throw while swapping/moving than `EC`, but it might be quite the opposite. Also, it is possible that while reswapping, another exception will be thrown. In general, you cannot guarantee the roll-back, so maybe it would be cleaner for everyone if you just declared that upon throw from swap, one cannot rely on the state of `result`: it should be reset or destroyed.
So, don't bother attempting to restore a valid state at all?
Does everybody else agree?
While it's true that EC could throw on swap, it's probably not very likely given that EC is most likely to be either error_code (or moral equivalent) or a simple enum/int. So that should be reasonably safe in the majority of cases.
Swapping back noexcept(false) T might be more problematic; successful swap out is not a guarantee of successful swap back.
So, maybe provide two implementations of swap: one for `EC` with no-throw swap, and the other for `EC` with throwing swap.
So, maybe provide two implementations of swap: one for `EC` with no-throw swap, and the other for `EC` with throwing swap.
That goes without saying. If `EC` is nothrow move and swap, there will be an optimised implementation in the post-review fixes. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2018-01-22 19:31 GMT-03:00 Gavin Lambert via Boost <boost@lists.boost.org>:
On 23/01/2018 03:53, Niall Douglas wrote:
My recommendation would be not to provide comparison operators if either
`T` or `EC` does not provide them.
Agreed. Logged to https://github.com/ned14/outcome/issues/107
I assume this would be individually for each case?
There is still benefit in providing operator==(EC) even if T does not provide operator==, eg. for the construct:
if (outcome == make_error_code(xxx))
(not sure if this is the exact spelling, but it's something like this)
Especially in generic code templated on T. The alternative is much more wordy and not really any clearer.
By alternative do you mean the following? if (outcome.error() == make_error_code(xxx)) I don't find it that much verbose and prefer to stick to Andrzej's suggestion on only providing these operators conditionally. -- Vinícius dos Santos Oliveira https://vinipsmaker.github.io/
participants (5)
-
Andrzej Krzemienski -
Gavin Lambert -
Niall Douglas -
Peter Dimov -
Vinícius dos Santos Oliveira