Repository: bitcoin/bitcoin
File: src/util/expected.h (C/C++)

Code before:
/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    using ValueType = std::conditional_t<std::is_same_v<T, void>, std::monostate, T>;
    std::variant<ValueType, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index_t<0>{}, ValueType{}} {}
    constexpr Expected(ValueType v) : m_data{std::in_place_index_t<0>{}, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index_t<1>{}, std::move(u.err)}
    {
    }

    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
    constexpr explicit operator bool() const noexcept { return has_value(); }

    constexpr const ValueType& value() const LIFETIMEBOUND
    {
        if (!Assume(has_value())) {
            throw BadExpectedAccess{};
        }
        return std::get<0>(m_data);
    }
    constexpr ValueType& value() LIFETIMEBOUND
    {
        if (!Assume(has_value())) {
            throw BadExpectedAccess{};
        }
        return std::get<0>(m_data);
    }

    template <class U>
    ValueType value_or(U&& default_value) const&
    {
        return has_value() ? value() : std::forward<U>(default_value);
    }
    template <class U>
    ValueType value_or(U&& default_value) &&
    {
        return has_value() ? std::move(value()) : std::forward<U>(default_value);
    }

    constexpr const E& error() const LIFETIMEBOUND
    {

Review comment (bug):
In commit "util: Make Expected::value() throw" (fa0297540e29f077a76133d1b60d3fd35f03b4e1)


In the standard `std::expected` class there is a clear difference between the high-level `value()` method which throws an exception when the value is unset, and lower level pointer operators which are `noexcept` and can't be called without a value. This makes `std::expected` consistent with `std::optional`, which also has `noexcept` pointer operators and a throwing `value()` method. It's also similar to `std::vector` which has nonthrowing `operator[]` methods and throwing `at()` methods.

I don't see a reason why `util::Expected` class shouldn't follow the same pattern and have `noexcept` pointer operators, especially if it's intended to be replaced with `std::expected` in the future. I also think it would clean up the implementation to have a separation between low level pointer operators and higher level accessor methods. Would suggest:

```c++
template <class T, class E>
class Expected
{
private:
    std::variant<T, E> m_data;

public:
    // Value constructors.
    constexpr Expected() = default;
    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}

    // Error constructor.
    template <class G>
    constexpr Expected(Unexpected<G> u) : m_data{std::in_place_index<1>, std::move(u).error()} {}

    // Value accessors.
    constexpr explicit operator bool() const noexcept { return m_data.index() == 0; }
    constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<0>(&m_data)); }
    constexpr T&       operator*() &      noexcept LIFETIMEBOUND { return *Assert(std::get_if<0>(&m_data)); }
    constexpr T&&      operator*() &&     noexcept LIFETIMEBOUND { return std::move(**this); }
    constexpr const T* operator->() const noexcept LIFETIMEBOUND { return Assert(std::get_if<0>(&m_data)); }
    constexpr T*       operator->()       noexcept LIFETIMEBOUND { return Assert(std::get_if<0>(&m_data)); }

    // Error accessors.
    constexpr const E& error() const& LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
    constexpr E& error() & LIFETIMEBOUND            { return *Assert(std::get_if<1>(&m_data)); }
    constexpr E&& error() && LIFETIMEBOUND          { return std::move(this->error()); }

    // Higher level helpers.
    constexpr bool has_value() const noexcept       { return bool{*this}; }
    constexpr const T& value() const& LIFETIMEBOUND { if (!*this) throw BadExpectedAccess{}; return **this; }
    constexpr T& value() & LIFETIMEBOUND            { if (!*this) throw BadExpectedAccess{}; return **this; }
    constexpr T&& value() && LIFETIMEBOUND          { return std::move(this->value()); }

    template <class U>
    T value_or(U&& default_value) const&
    {
        return *this ? **this : std::forward<U>(default_value);
    }

    template <class U>
    T value_or(U&& default_value) &&
    {
        return *this ? std::move(**this) : std::forward<U>(default_value);
    }
};
```

This implementation would also make the `value()` methods throw in debug builds instead of aborting, which I think is good because `std::expected` does that, and there isn't much reason to call `value()` if exceptions are unwanted. But no strong opinion about this aspect, and this could be easily tweaked to make `value()` abort in debug builds by replacing `if (!*this)` with `if (!Assume(*this))` in the two `value()` methods.

Code after:

struct BadExpectedAccess : std::exception {
    const char* what() const noexcept override { return "Bad util::Expected access"; }
};

/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    std::variant<T, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index<0>, T{}} {}
    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
    {
    }

    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
    constexpr explicit operator bool() const noexcept { return has_value(); }

    constexpr const T& value() const& LIFETIMEBOUND
    {
        if (!has_value()) {
            throw BadExpectedAccess{};
        }
        return std::get<0>(m_data);
    }
    constexpr T& value() & LIFETIMEBOUND
    {
        if (!has_value()) {
            throw BadExpectedAccess{};
        }
        return std::get<0>(m_data);
    }
    constexpr T&& value() && LIFETIMEBOUND { return std::move(value()); }

    template <class U>
    T value_or(U&& default_value) const&
    {
        return has_value() ? value() : std::forward<U>(default_value);
    }
    template <class U>
    T value_or(U&& default_value) &&
    {