util: Add some more Unexpected and Expected methods #34032

maflcko · 2025-12-09T08:01:34Z

Reviewers requested more member functions In #34006.

They are currently unused, but bring the port closer to the original std::expected implementation:

Make Expected::value() throw when no value exists
Add Unexpected::error() methods
Add Expected<void, E> specialization
Add Expected::value()&& and Expected::error()&& methods
Add Expected::swap()

Also, include a tiny tidy fixup:

tidy: Set AllowCastToVoid in the bugprone-unused-return-value check

DrahtBot · 2025-12-09T08:01:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34032.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator
Stale ACK	ryanofsky

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

src/util/expected.h

ryanofsky

Code review ACK fa20a50. Thanks for the followups.

I'd probably replace std::variant<std::monostate, E> with std::optional<E> but both seem reasonable.

maflcko · 2025-12-09T14:00:08Z

I'd probably replace std::variant<std::monostate, E> with std::optional<E> but both seem reasonable.

I kept the variant, because it is more consistent and shows that the error() methods are just 1:1 copy-pasted between both templates.

maflcko · 2025-12-09T16:38:26Z

pushed a commit, so that the two compile and behave identically:

    const auto moved{*std::move(no_copy)};
    const auto moved{std::move(*no_copy)};

Sorry for missing it earlier, but I think now all && methods are implemented completely and correctly.

hodlinator

ACK fabef9b

(Caveat: Not yet fully at ease with ampersands after the method parameters such as in the last two commits, example: constexpr E&& error() &&).

src/test/util_expected_tests.cpp

src/util/expected.h

ryanofsky

Code review ACK fa0a1a6. Left some suggestions which I think would make this better in significant ways, but I do this is already an improvement in its current form.

ryanofsky · 2025-12-10T18:03:07Z

src/util/expected.h

    constexpr const ValueType& value() const LIFETIMEBOUND
    {
-        assert(has_value());
+        if (!Assume(has_value())) {


In commit "util: Make Expected::value() throw" (fa02975)

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:

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.

Agree on change to use the default ctor, variant will initialize with the first type (https://en.cppreference.com/w/cpp/utility/variant/variant.html, 1).

I prefer the current way of implementing operator bool in terms of has_value() though, think it's clearer.

Agree on making the other operators noexcept.

Agree on making the other operators noexcept.

thx, done

Agree on change to use the default ctor, variant will initialize with the first type (https://en.cppreference.com/w/cpp/utility/variant/variant.html, 1).

shouldn't hurt to be explicit

ryanofsky · 2025-12-10T18:07:13Z

src/util/expected.h

+    constexpr E&& error() && LIFETIMEBOUND { return std::move(m_err); }
+
+private:
+    E m_err;


In commit "util: Add Unexpected::error()" (faa0d23)

Calling this m_error would seem more consistent.

ryanofsky · 2025-12-10T18:27:57Z

src/util/expected.h

+template <class E>
+class Expected<void, E>


In commit "util: Add Expected<void, E> specialization" (fa3d6c7)

This specialization is more complicated than it needs to be. Would suggest simplifying with inheritance:

//! Specialization for void returning void from value methods. template <class E> class Expected<void, E> : public Expected<std::monostate, E> { using Base = Expected<std::monostate, E>; public: using Expected<std::monostate, E>::Expected; constexpr void operator*() const noexcept { Base::operator*(); } constexpr void value() const& { Base::value(); } constexpr void value() && { Base::value(); } void value_or() = delete; };

This just brings back the problem we are trying to solve: Solve exposing monostate externally?

+ Expected<void, std::string> e{std::monostate{}};

It only makes sense to turn this off with C++26, which introduces the _ placeholder.

hodlinator

re-ACK fa0a1a6

hodlinator · 2025-12-11T08:42:40Z

src/util/expected.h

    constexpr const ValueType& value() const LIFETIMEBOUND
    {
-        assert(has_value());
+        if (!Assume(has_value())) {


Agree on change to use the default ctor, variant will initialize with the first type (https://en.cppreference.com/w/cpp/utility/variant/variant.html, 1).

I prefer the current way of implementing operator bool in terms of has_value() though, think it's clearer.

Agree on making the other operators noexcept.

src/util/expected.h

src/test/util_expected_tests.cpp

src/util/expected.h

This is not needed, but a bit closer to the std lib.

This is not expected to be needed in this codebase, but brings the implementation closer to std::expected::value().

This is not needed, but a bit closer to the std lib, because std::monostate is no longer leaked through ValueType from the value() method.

They are currently unused, but implementing them is closer to the std::expected.

It is currently unused, but implementing it is closer to std::expected.

hodlinator

re-ACK fa874c9

Thanks for grabbing swap!

hodlinator · 2025-12-11T10:23:29Z

src/util/expected.h

-    constexpr const E& error() const LIFETIMEBOUND
+    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()); }


How come this method overload is not calling itself recursively?

Tested adding BOOST_CHECK_EQUAL((Expected<void, int>{Unexpected{1}}.error()), 1); to the tests to confirm it doesn't but don't understand why.

Is this in some kind of &-mode, making it call the method above?

Is this in some kind of &-mode, making it call the method above?

It is not this, but (*this), see https://eel.is/c++draft/expr.prim#id.general-2

Can't read that. 😁

Can't read that. 😁

i'd say it says that error() in this context is (*this).error(). So it calls error()& and not error()&&. If you want to (incorrectly) recursively call error(), you'd have to add a cast: std::move(*this).error();

hodlinator · 2025-12-11T10:27:33Z

src/util/expected.h

+    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()); }
+
+    constexpr T* operator->() noexcept LIFETIMEBOUND { return &value(); }
+    constexpr const T* operator->() const noexcept LIFETIMEBOUND { return &value(); }


nit: Might be nice to have these assert before failing due to an exception being thrown inside of noexcept.

Why? Just seems more code for no reason?

It's about failing with a clearer stack trace/message. An exception will unwind the stack before giving a stack-trace (depending upon how the compiler treats exceptions within noexcept?).

Tried adding *e in the unit tests and it doesn't pinpoint the offending line, but gets by on checkpoints.

₿ ./build/bin/test_bitcoin -t util_expected_tests Running 9 test cases... terminate called after throwing an instance of 'util::BadExpectedAccess' what(): Bad util::Expected access unknown location(0): fatal error: in "util_expected_tests/expected_value_throws": signal: SIGABRT (application abort requested) ../src/test/util_expected_tests.cpp(73): last checkpoint

Yes, this is also the behavior that you get in C++23 (if you are lucky). If you are unlucky, you'll just get the silent UB:

https://godbolt.org/z/jYxqvoejn

/../include/c++/v1/__expected/expected.h:811: libc++ Hardening assertion this->__has_val() failed: expected::operator* requires the expected to contain a value Program terminated with signal: SIGSEGV

Also, this is the behavior with an assert/abort, so I don't see the difference!?

Yeah, we don't need to spoil ourselves before the switcharoo to std::expected. :)

DrahtBot added the Utils/log/libs label Dec 9, 2025

maflcko mentioned this pull request Dec 9, 2025

Add util::Expected (std::expected) #34006

Merged

ryanofsky reviewed Dec 9, 2025

View reviewed changes

src/util/expected.h Show resolved Hide resolved

ryanofsky approved these changes Dec 9, 2025

View reviewed changes

hodlinator approved these changes Dec 10, 2025

View reviewed changes

src/test/util_expected_tests.cpp Outdated Show resolved Hide resolved

src/util/expected.h Outdated Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

DrahtBot requested a review from ryanofsky December 10, 2025 15:42

maflcko force-pushed the 2512-exp branch from fabef9b to fa0a1a6 Compare December 10, 2025 16:29

ryanofsky approved these changes Dec 10, 2025

View reviewed changes

DrahtBot requested a review from hodlinator December 10, 2025 18:33

hodlinator mentioned this pull request Dec 11, 2025

rest: allow reading partial block data from storage #33657

Open

MarcoFalke added 2 commits December 11, 2025 09:46

Set bugprone-unused-return-value.AllowCastToVoid

fad4a9f

It only makes sense to turn this off with C++26, which introduces the _ placeholder.

test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK ==

faa109f

hodlinator reviewed Dec 11, 2025

View reviewed changes

MarcoFalke added 2 commits December 11, 2025 10:27

util: Add Unexpected::error()

fa1de11

This is not needed, but a bit closer to the std lib.

util: Make Expected::value() throw

fa5a49c

This is not expected to be needed in this codebase, but brings the implementation closer to std::expected::value().

maflcko force-pushed the 2512-exp branch from fa0a1a6 to fa6fcae Compare December 11, 2025 09:30

MarcoFalke added 4 commits December 11, 2025 10:38

util: Add Expected<void, E> specialization

fa696c4

This is not needed, but a bit closer to the std lib, because std::monostate is no longer leaked through ValueType from the value() method.

util: Implement Expected::value()&& and Expected::error()&&

fa71c13

They are currently unused, but implementing them is closer to the std::expected.

util: Implement Expected::operator*()&&

fa9aad7

It is currently unused, but implementing it is closer to std::expected.

util: Add Expected::swap()

fa874c9

maflcko force-pushed the 2512-exp branch from fa6fcae to fa874c9 Compare December 11, 2025 09:47

DrahtBot added the CI failed label Dec 11, 2025

hodlinator approved these changes Dec 11, 2025

View reviewed changes

DrahtBot requested a review from ryanofsky December 11, 2025 10:43

DrahtBot removed the CI failed label Dec 11, 2025

util: Add some more Unexpected and Expected methods #34032

Are you sure you want to change the base?

util: Add some more Unexpected and Expected methods #34032

Conversation

maflcko commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 9, 2025

Uh oh!

maflcko commented Dec 9, 2025

Uh oh!

hodlinator left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

maflcko commented Dec 9, 2025 •

edited

Loading

DrahtBot commented Dec 9, 2025 •

edited

Loading

hodlinator left a comment •

edited

Loading

maflcko Dec 11, 2025 •

edited

Loading