[boost] [outcome] comments on Regular ops

Subject: [boost] [outcome] comments on Regular ops
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2018-01-22 13:59:26

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;