Reviewers requested more member functions In #34006.
They are currently unused, but bring the port closer to the original std::expected implementation:
Expected::value() throw when no value existsUnexpected::error() methodsExpected<void, E> specializationExpected::value()&& and Expected::error()&& methodsExpected::swap()Also, include a tiny tidy fixup:
AllowCastToVoid in the bugprone-unused-return-value check
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34032.
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 <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
setup steps in nanobench by l0rinc)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.
81 }
82 template <class U>
83- ValueType value_or(U&& default_value) &&
84+ T value_or(U&& default_value) &&
85 {
86 return has_value() ? std::move(value()) : std::forward<U>(default_value);
std::move(value()) to value() since it should already be an rvalue, and this would make the value_or implementation more consistent
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.
I’d probably replace
std::variant<std::monostate, E>withstd::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.
pushed a commit, so that the two compile and behave identically:
0 const auto moved{*std::move(no_copy)};
1 const auto moved{std::move(*no_copy)};
Sorry for missing it earlier, but I think now all && methods are implemented completely and correctly.
nit: Could use BOOST_CHECK_EQUAL when applicable:
0 BOOST_CHECK_EQUAL(e.value().x, 42);
26+ constexpr const E& error() const& LIFETIMEBOUND { return err; }
27+ constexpr E& error() & LIFETIMEBOUND { return err; }
28+ constexpr E&& error() && LIFETIMEBOUND { return std::move(err); }
29+
30+private:
31 E err;
err -> m_err for consistency with Expected::m_data.
132+
133+template <class E>
134+class Expected<void, E>
135+{
136+private:
137+ std::variant<std::monostate, E> m_data;
nit: Could use Ah, I see that was already discussed #34032 (comment).optional instead of monostate?
0--- a/src/util/expected.h
1+++ b/src/util/expected.h
2@@ -10,6 +10,7 @@
3
4 #include <cassert>
5 #include <exception>
6+#include <optional>
7 #include <utility>
8 #include <variant>
9
10@@ -119,16 +120,16 @@ template <class E>
11 class Expected<void, E>
12 {
13 private:
14- std::variant<std::monostate, E> m_data;
15+ std::optional<E> m_error;
16
17 public:
18- constexpr Expected() : m_data{std::in_place_index<0>, std::monostate{}} {}
19+ constexpr Expected() = default;
20 template <class Err>
21- constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
22+ constexpr Expected(Unexpected<Err> u) : m_error{std::move(u).error()}
23 {
24 }
25
26- constexpr bool has_value() const noexcept { return m_data.index() == 0; }
27+ constexpr bool has_value() const noexcept { return !m_error.has_value(); }
28 constexpr explicit operator bool() const noexcept { return has_value(); }
29
30 constexpr void value() const
31@@ -141,17 +142,17 @@ public:
32 constexpr const E& error() const& LIFETIMEBOUND
33 {
34 assert(!has_value());
35- return std::get<1>(m_data);
36+ return m_error.value();
37 }
38 constexpr E& error() & LIFETIMEBOUND
39 {
40 assert(!has_value());
41- return std::get<1>(m_data);
42+ return m_error.value();
43 }
44 constexpr E&& error() && LIFETIMEBOUND
45 {
46 assert(!has_value());
47- return std::get<1>(std::move(m_data));
48+ return std::move(m_error).value();
49 }
50 };
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() &&).
54@@ -49,12 +55,16 @@ class Expected
55
56 constexpr const ValueType& value() const LIFETIMEBOUND
57 {
58- assert(has_value());
59+ if (!Assume(has_value())) {
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:
0template <class T, class E>
1class Expected
2{
3private:
4 std::variant<T, E> m_data;
5
6public:
7 // Value constructors.
8 constexpr Expected() = default;
9 constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
10
11 // Error constructor.
12 template <class G>
13 constexpr Expected(Unexpected<G> u) : m_data{std::in_place_index<1>, std::move(u).error()} {}
14
15 // Value accessors.
16 constexpr explicit operator bool() const noexcept { return m_data.index() == 0; }
17 constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<0>(&m_data)); }
18 constexpr T& operator*() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<0>(&m_data)); }
19 constexpr T&& operator*() && noexcept LIFETIMEBOUND { return std::move(**this); }
20 constexpr const T* operator->() const noexcept LIFETIMEBOUND { return Assert(std::get_if<0>(&m_data)); }
21 constexpr T* operator->() noexcept LIFETIMEBOUND { return Assert(std::get_if<0>(&m_data)); }
22
23 // Error accessors.
24 constexpr const E& error() const& LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
25 constexpr E& error() & LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
26 constexpr E&& error() && LIFETIMEBOUND { return std::move(this->error()); }
27
28 // Higher level helpers.
29 constexpr bool has_value() const noexcept { return bool{*this}; }
30 constexpr const T& value() const& LIFETIMEBOUND { if (!*this) throw BadExpectedAccess{}; return **this; }
31 constexpr T& value() & LIFETIMEBOUND { if (!*this) throw BadExpectedAccess{}; return **this; }
32 constexpr T&& value() && LIFETIMEBOUND { return std::move(this->value()); }
33
34 template <class U>
35 T value_or(U&& default_value) const&
36 {
37 return *this ? **this : std::forward<U>(default_value);
38 }
39
40 template <class U>
41 T value_or(U&& default_value) &&
42 {
43 return *this ? std::move(**this) : std::forward<U>(default_value);
44 }
45};
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,
variantwill initialize with the first type (https://en.cppreference.com/w/cpp/utility/variant/variant.html, 1).
shouldn’t hurt to be explicit
29+ constexpr const E& error() const& LIFETIMEBOUND { return m_err; }
30+ constexpr E& error() & LIFETIMEBOUND { return m_err; }
31+ constexpr E&& error() && LIFETIMEBOUND { return std::move(m_err); }
32+
33+private:
34+ E m_err;
In commit “util: Add Unexpected::error()” (faa0d23ee75aac289c3766ce6ee27948b0d982d9)
Calling this m_error would seem more consistent.
104+ constexpr T* operator->() LIFETIMEBOUND { return &value(); }
105+ constexpr const T* operator->() const LIFETIMEBOUND { return &value(); }
106+};
107+
108+template <class E>
109+class Expected<void, E>
In commit “util: Add Expected<void, E> specialization” (fa3d6c760db3dafbaa530401893a40fc5f6575f9)
This specialization is more complicated than it needs to be. Would suggest simplifying with inheritance:
0//! Specialization for void returning void from value methods.
1template <class E>
2class Expected<void, E> : public Expected<std::monostate, E>
3{
4 using Base = Expected<std::monostate, E>;
5public:
6 using Expected<std::monostate, E>::Expected;
7 constexpr void operator*() const noexcept { Base::operator*(); }
8 constexpr void value() const& { Base::value(); }
9 constexpr void value() && { Base::value(); }
10 void value_or() = delete;
11};
This just brings back the problem we are trying to solve: Solve exposing monostate externally?
+ Expected<void, std::string> e{std::monostate{}};
re: #34032 (review)
This just brings back the problem we are trying to solve
I don’t think it does. The problem I want the void specialization to solve is leaking the std::monostate values to code that isn’t otherwise using it and is expecting void, making it harder to use std::expected in places like template functions or generic lambdas. I don’t see std::expected accepting (not returning) std::monostate{} as a comparable problem, or as a real problem, even though it could be avoided by declaring a deleted constructor. The current approach does seem ok though, just repetitive.
It only makes sense to turn this off with C++26, which introduces the _
placeholder.
167+ constexpr E&& error() && LIFETIMEBOUND
168+ {
169+ assert(!has_value());
170+ return std::get<1>(std::move(m_data));
171+ }
172 };
nit: Would be nice to add swap() as well. I already needed that here: #25665 (review) / https://github.com/hodlinator/bitcoin/commit/b40a36cbbab59440d84dba6d1cc16bce17d4869c
(I forgot to include the test in that commit).
0BOOST_AUTO_TEST_CASE(expected_swap)
1{
2 Expected<const char*, int> a{Unexpected{55}};
3 Expected<const char*, int> b{"21"};
4 a.swap(b);
5 BOOST_CHECK_EQUAL(a.value(), "21");
6 BOOST_CHECK_EQUAL(b.error(), 55);
7}
21@@ -17,25 +22,39 @@ BOOST_AUTO_TEST_CASE(expected_value)
22 int x;
23 };
24 Expected<Obj, int> e{};
25- BOOST_CHECK(e.value().x == 0);
26+ BOOST_CHECK_EQUAL(e.value().x, 0);
27
28 e = Obj{42};
29
30 BOOST_CHECK(e.has_value());
nit: Should this be a require since otherwise we would start triggering internal asserts or exceptions further down on failure?
0 BOOST_REQUIRE(e.has_value());
25- E err;
26+ constexpr explicit Unexpected(E e) : m_err(std::move(e)) {}
27+
28+ constexpr const E& error() const& LIFETIMEBOUND { return m_err; }
29+ constexpr E& error() & LIFETIMEBOUND { return m_err; }
30+ constexpr E&& error() && LIFETIMEBOUND { return std::move(m_err); }
noexcept according to https://en.cppreference.com/w/cpp/utility/expected/unexpected.html#error
This is not needed, but a bit closer to the std lib.
96 }
97
98- constexpr const E& error() const LIFETIMEBOUND
99+ constexpr const E& error() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
100+ constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
101+ 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
thisin 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. 😁
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();
105+ constexpr T& operator*() & noexcept LIFETIMEBOUND { return value(); }
106+ constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return value(); }
107+ constexpr T&& operator*() && noexcept LIFETIMEBOUND { return std::move(value()); }
108+
109+ constexpr T* operator->() noexcept LIFETIMEBOUND { return &value(); }
110+ constexpr const T* operator->() const noexcept LIFETIMEBOUND { return &value(); }
noexcept.
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.
0₿ ./build/bin/test_bitcoin -t util_expected_tests
1Running 9 test cases...
2terminate called after throwing an instance of 'util::BadExpectedAccess'
3 what(): Bad util::Expected access
4unknown location(0): fatal error: in "util_expected_tests/expected_value_throws": signal: SIGABRT (application abort requested)
5../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
0/../include/c++/v1/__expected/expected.h:811: libc++ Hardening assertion this->__has_val() failed: expected::operator* requires the expected to contain a value
1Program terminated with signal: SIGSEGV
Also, this is the behavior with an assert/abort, so I don’t see the difference!?
std::expected. :)
re-ACK fa874c9f050500dc521ef8d9c81b043687f0dcc7
Thanks for grabbing swap!
68 }
69 constexpr ValueType& value() LIFETIMEBOUND
70 {
71- assert(has_value());
72+ if (!has_value()) {
73+ 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 (review) 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 useoperator*instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap theutil::Expectedclass withstd::expectedin 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:
0valgrind --tool=none ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no
1==1207793== Nulgrind, the minimal Valgrind tool
2...
3==1207793== Command: ./bld-cmake/bin/test_bitcoin -t util_expected_tests/expected_error --catch_system_errors=no
4==1207793==
5Running 1 test case...
6terminate called after throwing an instance of 'util::BadExpectedAccess'
7 what(): Bad util::Expected access
8==1207793==
9==1207793== Process terminating with default action of signal 6 (SIGABRT): dumping core
10...
11==1207793== by 0x4F1A98A: util_expected_tests::expected_error::test_method() (expected.h:117)
12==1207793== by 0x4F19B56: util_expected_tests::expected_error_invoker() (./test/util_expected_tests.cpp:79)
13...
14Aborted (core dumped)
I think this is perfectly fine, and I don’t see the benefit of the assertion.
94+ constexpr const T& operator*() const& noexcept LIFETIMEBOUND { return value(); }
95+ constexpr T&& operator*() && noexcept LIFETIMEBOUND { return std::move(value()); }
96
97- constexpr T* operator->() LIFETIMEBOUND { return &value(); }
98- constexpr const T* operator->() const LIFETIMEBOUND { return &value(); }
99+ 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
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.
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.
98- constexpr const E& error() const LIFETIMEBOUND
99+ constexpr const E& error() const& noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
100+ constexpr E& error() & noexcept LIFETIMEBOUND { return *Assert(std::get_if<1>(&m_data)); }
101+ constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(error()); }
102+
103+ constexpr void swap(Expected& other) noexcept { m_data.swap(other.m_data); }
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.
re-ACK faebb1654743adcc5888464f9a7167c315ecce1f
Only improved commit messages and made operator* for Expected<void, E>-specialization throw on error since my previous review (https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3566738644)