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 | ryanofsky, stickies-v, hodlinator, sedited |
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:
spend coverage by Chand-ra)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.
@ryanofsky No rush, but if you get a chance to take another look here before branch-off [1], that’d be great. I think the pull is currently blocked because of your use of the spicy word “unsafe” :sweat_smile:
[1] It would be good to get this in before branch-off. Otherwise, there could be (unlikely) compile-failures or silent performance regressions for backports due to #34032 (comment)
re: #34032 (review)
@ryanofsky No rush, but if you get a chance to take another look here before branch-off [1], that’d be great. I think the pull is currently blocked because of your use of the spicy word “unsafe” 😅
I’m not saying the pull is unsafe. I’m saying this specific commit fa90c4434176860eb01c8197fda226f6e7bb1f70 is unsafe and the updated commit message is misleading.
This commit is unsafe and the new commit message saying that pointer operators throwing exceptions is “fine” is misleading, because if these operators throw exceptions, code can catch those exceptions, and that code will be broken when Expected is replaced with expected, in bad ways triggering UB, in code paths likely to be missing test coverage because we often don’t have tests checking error paths.
However, the pull request as a whole is safe because a later commit fa15f71e34e53ecaa32c4b4f2102feed7601c62c adds noexcept. That later commit fixes safety issue apparently by accident because it does not even mention any change in behavior.
IMO, this pull is not in a good state even if it is technically safe. These issues could be fixed at a surface level by adding noexcept in fa90c4434176860eb01c8197fda226f6e7bb1f70 instead of fa15f71e34e53ecaa32c4b4f2102feed7601c62c. In that case, it would be good to have comments next to these noexcepts, because normally when developers see noexcept we think “exceptions are not expected to be thrown here” not “exceptions are thrown one level down from here but this triggers std::terminate and that is good.”
I do think the real mistake here is implementing pointer methods in terms of value() instead of the other way around. Using value() complicates implementation of this class, makes error reporting more confusing and less reliable, and sets a bad example of how you should deal with nullable values in c++. Using value instead of bool/*/-> makes unsafe dereferences look like normal method calls, and negates decades of pattern recognition c/c++ developers have dealing with code with nullable values.
Thanks for the reply. I think I understand your point better now.
These issues could be fixed at a surface level by adding noexcept in https://github.com/bitcoin/bitcoin/commit/fa90c4434176860eb01c8197fda226f6e7bb1f70 instead of https://github.com/bitcoin/bitcoin/commit/fa15f71e34e53ecaa32c4b4f2102feed7601c62c.
Thanks, done.
normally when developers see noexcept we think “exceptions are not expected to be thrown here” not “exceptions are thrown one level down from here but this triggers std::terminate and that is good.”
I think the first interpretation is correct, because no code should exist that dereferences a nullopt, so I’ll not add a comment for now.
I do think the real mistake here is implementing pointer methods in terms of
value()instead of the other way around. Usingvalue()complicates implementation of this class, makes error reporting more confusing and less reliable, and sets a bad example of how you should deal with nullable values in c++. Usingvalueinstead ofbool/*/->makes unsafe dereferences look like normal method calls, and negates decades of pattern recognition c/c++ developers have dealing with code with nullable values.
I guess I just disagree here.
Assert has a nicer error message here than terminate. See my prior reply.
re: #34032 (review)
- I don’t like UB and I think we should move toward eliminating it, so I am not going to re-write this as unsafe code.
If you think something in the suggestion #34032 (review) is unsafe, it would help to clarify what you are referring to.
Just because C++ is know for its UB for decades doesn’t mean all future code needs to be unsafe as well.
We have no disagreement about this. I am not saying it is bad to use value() in general. I think it is great to use value() in code that catches exceptions. However, it is bad to use value() in code that does not catch exceptions because:
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.
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)
This is not expected to be needed in this codebase, but brings the
implementation closer to std::expected::value().
Also, add noexcept, where std::expected has them. This will make
operator-> and operator* terminate, when 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.
Force pushed something. Should be trivial to re-review, because nothing changed:
0$ git diff faebb16547 faa59b3679 | wc -l
10
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Range-diff shows noexcept being introduced earlier. Diff against last reviewed push faebb1654743adcc5888464f9a7167c315ecce1f confirms end state is the same.
5@@ -6,9 +6,10 @@
6 #define BITCOIN_UTIL_EXPECTED_H
7
8 #include <attributes.h>
9+#include <util/check.h>
10
11 #include <cassert>
yeah, i am not sure which way to go. The export pragma in
0src/util/check.h:#include <cassert> // IWYU pragma: export
was added so that iwyu does not remove the
0#if defined(NDEBUG)
1#error "Cannot compile without assertions!"
2#endif
later in the header (by removing the inclusion of the header).
However, that seems a bit incomplete and brittle. It would be better to go fully in one direction, to either:
Not sure which way to go, though