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 | 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.
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{}};
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.
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.
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!