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	stickies-v, hodlinator
Stale ACK	ryanofsky

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34264 (fuzz: Extend spend coverage by Chand-ra)
#34208 (bench: add fluent API for untimed setup steps in nanobench by l0rinc)
#33689 (http: replace WorkQueue and single threads handling for ThreadPool by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/util/expected.h

ryanofsky

Code review ACK fa20a50f46307b45a82a6e4283a945ef2f9f92a2. 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 fabef9bbef255796f28c04c5b478df9544c749eb

(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 fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb. 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.

src/util/expected.h

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

hodlinator

re-ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb

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.

hodlinator

re-ACK fa874c9f050500dc521ef8d9c81b043687f0dcc7

Thanks for grabbing swap!

src/util/expected.h

ryanofsky

Code review fa874c9f050500dc521ef8d9c81b043687f0dcc7. Looks mostly good and thanks for considering the previous suggestions. One thing that I think may be a problem is use of throwing value() method in operator methods declared noexcept but I left a more detailed comment below.

Various changes were made since last review: renaming m_err, getting rid of Assume() in value(), testing void value() specialization, adding noexcepts, adding method & qualifiers, reformatting and using std::get_if, adding swap method.

ryanofsky · 2025-12-17T18:41:07Z

src/util/expected.h

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


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 #34032 (comment) 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.

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 understand that switching util::Expected::operator* to std::expected::operator* is unsafe, but this is documented in https://en.cppreference.com/w/cpp/utility/expected/operator%252A.html:

If has_value() is false, the behavior is undefined. (until C++26)

Though, that is true with or without this pull request, so it seems unrelated.

As for this implementation: I think it is perfectly fine to do whatever on UB, and asserting, or throwing seems better than UB.

but asserts would probably lead to clearer runtime errors than noexcept violations.

I don't understand why that would be. The only difference with an assert is that it prints the file name and line number, but this will just be a inside this util header, not in the place in the code where the assertion happens. So reading the tb is needed anyway. For reference, with noexecpt, it looks like:

valgrind --tool=none ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no ==1207793== Nulgrind, the minimal Valgrind tool ... ==1207793== Command: ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no ==1207793== Running 1 test case... terminate called after throwing an instance of 'util::BadExpectedAccess' what(): Bad util::Expected access ==1207793== ==1207793== Process terminating with default action of signal 6 (SIGABRT): dumping core ... ==1207793== by 0x4F1A98A: util_expected_tests::expected_error::test_method() (expected.h:117) ==1207793== by 0x4F19B56: util_expected_tests::expected_error_invoker() (./test/util_expected_tests.cpp:79) ... Aborted (core dumped)

I think this is perfectly fine, and I don't see the benefit of the assertion.

src/util/expected.h

ryanofsky · 2025-12-17T19:01:48Z

src/util/expected.h


-    constexpr T* operator->() LIFETIMEBOUND { return &value(); }
-    constexpr const T* operator->() const LIFETIMEBOUND { return &value(); }
+    constexpr T* operator->() noexcept LIFETIMEBOUND { return &value(); }


In commit "util: Implement Expected::operator*()&&" (fa9aad738a9bfd3969aa4e1f19f87edadc69f455)

Would be good for commit message to mention it is also adding noexcept to other overloads

This is not expected to be needed in this codebase, but brings the implementation closer to std::expected::value(). Review note: This will also make operator-> and operator* throw, but this is fine, because calling those is UB when std::expected::has_value() is false.

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. Also, add noexcept, where std::expected has them.

stickies-v

utACK faebb16

stickies-v · 2026-01-05T09:08:47Z

src/util/expected.h

+    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); }


review note: std::variant::swap is not unconditionally noexcept but has a specification. However, I think for our minimal implementation just letting it std::terminate is fine.

hodlinator

re-ACK faebb16

Only improved commit messages and made operator* for Expected<void, E>-specialization throw on error since my previous review (#34032 (review))

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

src/util/expected.h 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 hodlinator December 10, 2025 18:33

hodlinator mentioned this pull request Dec 11, 2025

rest: allow reading partial block data from storage #33657

Merged

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

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

src/util/expected.h Show resolved Hide resolved

src/test/util_expected_tests.cpp Show resolved Hide resolved

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

util: Add Unexpected::error()

fa1de11

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

maflcko force-pushed the 2512-exp branch 2 times, most recently 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

src/util/expected.h Show resolved Hide resolved

src/util/expected.h Show resolved Hide resolved

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

DrahtBot removed the CI failed label Dec 11, 2025

maflcko requested a review from stickies-v December 15, 2025 11:14

ryanofsky reviewed Dec 17, 2025

View reviewed changes

DrahtBot requested a review from ryanofsky December 17, 2025 19:13

MarcoFalke added 4 commits December 18, 2025 08:58

util: Add Expected<void, E> specialization

fa31884

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()&&

fad1be9

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

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

fa15f71

It is currently unused, but implementing it is closer to std::expected. Also, add noexcept, where std::expected has them.

util: Add Expected::swap()

faebb16

maflcko force-pushed the 2512-exp branch from fa874c9 to faebb16 Compare December 18, 2025 08:01

DrahtBot mentioned this pull request Dec 29, 2025

http: replace WorkQueue and single threads handling for ThreadPool #33689

Open

stickies-v approved these changes Jan 5, 2026

View reviewed changes

DrahtBot requested a review from hodlinator January 5, 2026 09:11

maflcko requested review from ryanofsky and removed request for ryanofsky January 5, 2026 10:10

hodlinator approved these changes Jan 6, 2026

View reviewed changes

This was referenced Jan 6, 2026

bench: add fluent API for untimed setup steps in nanobench #34208

Open

fuzz: Extend spend coverage #34264

Open

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

Conflicts

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!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maflcko commented Dec 9, 2025 •

edited

Loading

DrahtBot commented Dec 9, 2025 •

edited

Loading

hodlinator left a comment •

edited

Loading