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

Code before:
{
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).error()}
    {
    }

    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 (!has_value()) {
            throw BadExpectedAccess{};
        }
        return std::get<0>(m_data);
    }
    constexpr ValueType& value() LIFETIMEBOUND
    {
        if (!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
    {
        assert(!has_value());
        return std::get<1>(m_data);
    }
    constexpr E& error() LIFETIMEBOUND
    {
        assert(!has_value());
        return std::get<1>(m_data);

Review comment (suggestion):
In commit "util: Make Expected::value() throw" (fa5a49c988c065526105f7d30c53298305f6e8c5)

The change seems unsafe because none of the other functions currently calling `value()` are switched to use `operator*` instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the `util::Expected` class with `std::expected` in the future.

I'd recommend reconsidering the suggestion from https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607689549 and avoiding unnecessary uses of `value()` and `has_value()` preferring to use c++'s built in operators, and setting a better example for other code to follow. Alternately more asserts could be added to fix this. This is also sort-of fixed in a later commit fa9aad738a9bfd3969aa4e1f19f87edadc69f455 adding noexcept to pointer operators, but asserts would probably lead to clearer runtime errors than noexcept violations.



Code after:
{
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) &&
    {
        return has_value() ? std::move(value()) : std::forward<U>(default_value);
    }

    constexpr const E& error() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
    constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(error()); }

    constexpr void swap(Expected& other) noexcept { m_data.swap(other.m_data); }

    constexpr T& operator*() & noexcept LIFETIMEBOUND { return value(); }
    constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return value(); }
    constexpr T&& operator*() && noexcept LIFETIMEBOUND { return std::move(value()); }