diff --git a/src/.clang-tidy b/src/.clang-tidy index a4378cea308..f54e07facae 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -41,3 +41,5 @@ CheckOptions: value: false - key: bugprone-unused-return-value.CheckedReturnTypes value: '^::std::error_code$;^::std::error_condition$;^::std::errc$;^::std::expected$;^::util::Result$;^::util::Expected$' + - key: bugprone-unused-return-value.AllowCastToVoid + value: true # Can be removed with C++26 once the _ placeholder exists. diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 186e16718c0..72b3d70ec68 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -130,7 +130,7 @@ static void BnBExhaustion(benchmark::Bench& bench) bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - [[maybe_unused]] auto _{SelectCoinsBnB(utxo_pool, target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT)}; // Should exhaust + (void)SelectCoinsBnB(utxo_pool, target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT); // Should exhaust // Cleanup utxo_pool.clear(); diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 6cf47eba736..71c6f5b1ee2 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -330,7 +330,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg) std::unique_ptr item(new HTTPWorkItem(std::move(hreq), path, i->handler)); assert(g_work_queue); if (g_work_queue->Enqueue(item.get())) { - [[maybe_unused]] auto _{item.release()}; /* if true, queue took ownership */ + (void)item.release(); /* if true, queue took ownership */ } else { LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting"); item->req->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded"); diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp index a8c28fdcfca..bec288813ad 100644 --- a/src/ipc/test/ipc_test.cpp +++ b/src/ipc/test/ipc_test.cpp @@ -63,9 +63,7 @@ void IpcPipeTest() auto foo_client = std::make_unique>( connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs(), connection_client.get(), /* destroy_connection= */ true); - { - [[maybe_unused]] auto _{connection_client.release()}; - } + (void)connection_client.release(); foo_promise.set_value(std::move(foo_client)); auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { diff --git a/src/test/util_expected_tests.cpp b/src/test/util_expected_tests.cpp index 61a6d2e0745..5979250ea90 100644 --- a/src/test/util_expected_tests.cpp +++ b/src/test/util_expected_tests.cpp @@ -7,6 +7,11 @@ #include +#include +#include +#include + + using namespace util; BOOST_AUTO_TEST_SUITE(util_expected_tests) @@ -17,15 +22,15 @@ BOOST_AUTO_TEST_CASE(expected_value) int x; }; Expected e{}; - BOOST_CHECK(e.value().x == 0); + BOOST_CHECK_EQUAL(e.value().x, 0); e = Obj{42}; BOOST_CHECK(e.has_value()); BOOST_CHECK(static_cast(e)); - BOOST_CHECK(e.value().x == 42); - BOOST_CHECK((*e).x == 42); - BOOST_CHECK(e->x == 42); + BOOST_CHECK_EQUAL(e.value().x, 42); + BOOST_CHECK_EQUAL((*e).x, 42); + BOOST_CHECK_EQUAL(e->x, 42); // modify value e.value().x += 1; @@ -33,9 +38,23 @@ BOOST_AUTO_TEST_CASE(expected_value) e->x += 1; const auto& read{e}; - BOOST_CHECK(read.value().x == 45); - BOOST_CHECK((*read).x == 45); - BOOST_CHECK(read->x == 45); + BOOST_CHECK_EQUAL(read.value().x, 45); + BOOST_CHECK_EQUAL((*read).x, 45); + BOOST_CHECK_EQUAL(read->x, 45); +} + +BOOST_AUTO_TEST_CASE(expected_value_rvalue) +{ + Expected, int> no_copy{std::make_unique(5)}; + const auto moved{std::move(no_copy).value()}; + BOOST_CHECK_EQUAL(*moved, 5); +} + +BOOST_AUTO_TEST_CASE(expected_deref_rvalue) +{ + Expected, int> no_copy{std::make_unique(5)}; + const auto moved{*std::move(no_copy)}; + BOOST_CHECK_EQUAL(*moved, 5); } BOOST_AUTO_TEST_CASE(expected_value_or) @@ -48,21 +67,68 @@ BOOST_AUTO_TEST_CASE(expected_value_or) BOOST_CHECK_EQUAL(const_val.value_or("fallback"), "fallback"); } +BOOST_AUTO_TEST_CASE(expected_value_throws) +{ + const Expected e{Unexpected{"fail"}}; + BOOST_CHECK_THROW(e.value(), BadExpectedAccess); + + const Expected void_e{Unexpected{"fail"}}; + BOOST_CHECK_THROW(void_e.value(), BadExpectedAccess); +} + BOOST_AUTO_TEST_CASE(expected_error) { Expected e{}; BOOST_CHECK(e.has_value()); + [&]() -> void { return e.value(); }(); // check value returns void and does not throw + [&]() -> void { return *e; }(); e = Unexpected{"fail"}; BOOST_CHECK(!e.has_value()); BOOST_CHECK(!static_cast(e)); - BOOST_CHECK(e.error() == "fail"); + BOOST_CHECK_EQUAL(e.error(), "fail"); // modify error e.error() += "1"; const auto& read{e}; - BOOST_CHECK(read.error() == "fail1"); + BOOST_CHECK_EQUAL(read.error(), "fail1"); +} + +BOOST_AUTO_TEST_CASE(expected_error_rvalue) +{ + { + Expected> nocopy_err{Unexpected{std::make_unique(7)}}; + const auto moved{std::move(nocopy_err).error()}; + BOOST_CHECK_EQUAL(*moved, 7); + } + { + Expected> void_nocopy_err{Unexpected{std::make_unique(9)}}; + const auto moved{std::move(void_nocopy_err).error()}; + BOOST_CHECK_EQUAL(*moved, 9); + } +} + +BOOST_AUTO_TEST_CASE(unexpected_error_accessors) +{ + Unexpected u{std::make_unique(-1)}; + BOOST_CHECK_EQUAL(*u.error(), -1); + + *u.error() -= 1; + const auto& read{u}; + BOOST_CHECK_EQUAL(*read.error(), -2); + + const auto moved{std::move(u).error()}; + BOOST_CHECK_EQUAL(*moved, -2); +} + +BOOST_AUTO_TEST_CASE(expected_swap) +{ + Expected> a{Unexpected{std::make_unique(-1)}}; + Expected> b{"good"}; + a.swap(b); + BOOST_CHECK_EQUAL(a.value(), "good"); + BOOST_CHECK_EQUAL(*b.error(), -1); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/expected.h b/src/util/expected.h index 0e7256f8677..66fb98e0558 100644 --- a/src/util/expected.h +++ b/src/util/expected.h @@ -6,9 +6,10 @@ #define BITCOIN_UTIL_EXPECTED_H #include +#include #include -#include +#include #include #include @@ -20,8 +21,18 @@ template class Unexpected { public: - constexpr explicit Unexpected(E e) : err(std::move(e)) {} - E err; + constexpr explicit Unexpected(E e) : m_error(std::move(e)) {} + + constexpr const E& error() const& noexcept LIFETIMEBOUND { return m_error; } + constexpr E& error() & noexcept LIFETIMEBOUND { return m_error; } + constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(m_error); } + +private: + E m_error; +}; + +struct BadExpectedAccess : std::exception { + const char* what() const noexcept override { return "Bad util::Expected access"; } }; /// The util::Expected class provides a standard way for low-level functions to @@ -33,58 +44,87 @@ template class Expected { private: - using ValueType = std::conditional_t, std::monostate, T>; - std::variant m_data; + std::variant m_data; public: - constexpr Expected() : m_data{std::in_place_index_t<0>{}, ValueType{}} {} - constexpr Expected(ValueType v) : m_data{std::in_place_index_t<0>{}, std::move(v)} {} + constexpr Expected() : m_data{std::in_place_index<0>, T{}} {} + constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {} template - constexpr Expected(Unexpected u) : m_data{std::in_place_index_t<1>{}, std::move(u.err)} + constexpr Expected(Unexpected u) : m_data{std::in_place_index<1>, std::move(u).error()} { } constexpr bool has_value() const noexcept { return m_data.index() == 0; } constexpr explicit operator bool() const noexcept { return has_value(); } - constexpr const ValueType& value() const LIFETIMEBOUND + constexpr const T& value() const& LIFETIMEBOUND { - assert(has_value()); + if (!has_value()) { + throw BadExpectedAccess{}; + } return std::get<0>(m_data); } - constexpr ValueType& value() LIFETIMEBOUND + constexpr T& value() & LIFETIMEBOUND { - assert(has_value()); + if (!has_value()) { + throw BadExpectedAccess{}; + } return std::get<0>(m_data); } + constexpr T&& value() && LIFETIMEBOUND { return std::move(value()); } template - ValueType value_or(U&& default_value) const& + T value_or(U&& default_value) const& { return has_value() ? value() : std::forward(default_value); } template - ValueType value_or(U&& default_value) && + T value_or(U&& default_value) && { return has_value() ? std::move(value()) : std::forward(default_value); } - 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()); } + + constexpr void swap(Expected& other) noexcept { m_data.swap(other.m_data); } + + 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(); } +}; + +template +class Expected +{ +private: + std::variant m_data; + +public: + constexpr Expected() : m_data{std::in_place_index<0>, std::monostate{}} {} + template + constexpr Expected(Unexpected u) : m_data{std::in_place_index<1>, std::move(u).error()} { - assert(!has_value()); - return std::get<1>(m_data); - } - constexpr E& error() LIFETIMEBOUND - { - assert(!has_value()); - return std::get<1>(m_data); } - constexpr ValueType& operator*() LIFETIMEBOUND { return value(); } - constexpr const ValueType& operator*() const LIFETIMEBOUND { return value(); } + constexpr bool has_value() const noexcept { return m_data.index() == 0; } + constexpr explicit operator bool() const noexcept { return has_value(); } - constexpr ValueType* operator->() LIFETIMEBOUND { return &value(); } - constexpr const ValueType* operator->() const LIFETIMEBOUND { return &value(); } + constexpr void operator*() const noexcept { return value(); } + constexpr void value() const + { + if (!has_value()) { + throw BadExpectedAccess{}; + } + } + + 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()); } }; } // namespace util diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp index 99bc5345a34..552364a667d 100644 --- a/src/wallet/test/fuzz/spend.cpp +++ b/src/wallet/test/fuzz/spend.cpp @@ -98,7 +98,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup) std::optional change_pos; if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral(); - [[maybe_unused]] auto _{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)}; + (void)CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control); } } // namespace } // namespace wallet