New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Replace BResult with util::Result #25721
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -88,7 +88,7 @@ class Wallet | |||||
virtual std::string getWalletName() = 0; | ||||||
|
||||||
// Get a new address. | ||||||
virtual BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0; | ||||||
virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While touching this line here and in the override function in
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Picked up in #25616. |
||||||
|
||||||
//! Get public key. | ||||||
virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0; | ||||||
|
@@ -139,7 +139,7 @@ class Wallet | |||||
virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0; | ||||||
|
||||||
//! Create transaction. | ||||||
virtual BResult<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients, | ||||||
virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients, | ||||||
const wallet::CCoinControl& coin_control, | ||||||
bool sign, | ||||||
int& change_pos, | ||||||
|
@@ -329,7 +329,7 @@ class WalletLoader : public ChainClient | |||||
virtual std::string getWalletDir() = 0; | ||||||
|
||||||
//! Restore backup wallet | ||||||
virtual BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0; | ||||||
virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0; | ||||||
|
||||||
//! Return available wallets in wallet directory. | ||||||
virtual std::vector<std::string> listWalletDir() = 0; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
// Copyright (c) 2022 The Bitcoin Core developers | ||
// Distributed under the MIT software license, see the accompanying | ||
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
||
#include <util/result.h> | ||
|
||
#include <boost/test/unit_test.hpp> | ||
|
||
inline bool operator==(const bilingual_str& a, const bilingual_str& b) | ||
{ | ||
return a.original == b.original && a.translated == b.translated; | ||
} | ||
|
||
inline std::ostream& operator<<(std::ostream& os, const bilingual_str& s) | ||
{ | ||
return os << "bilingual_str('" << s.original << "' , '" << s.translated << "')"; | ||
} | ||
|
||
BOOST_AUTO_TEST_SUITE(result_tests) | ||
|
||
struct NoCopy { | ||
NoCopy(int n) : m_n{std::make_unique<int>(n)} {} | ||
std::unique_ptr<int> m_n; | ||
}; | ||
|
||
bool operator==(const NoCopy& a, const NoCopy& b) | ||
{ | ||
return *a.m_n == *b.m_n; | ||
} | ||
|
||
std::ostream& operator<<(std::ostream& os, const NoCopy& o) | ||
{ | ||
return os << "NoCopy(" << *o.m_n << ")"; | ||
} | ||
|
||
util::Result<int> IntFn(int i, bool success) | ||
{ | ||
if (success) return i; | ||
return util::Error{Untranslated(strprintf("int %i error.", i))}; | ||
} | ||
|
||
util::Result<bilingual_str> StrFn(bilingual_str s, bool success) | ||
{ | ||
if (success) return s; | ||
return util::Error{strprintf(Untranslated("str %s error."), s.original)}; | ||
} | ||
|
||
util::Result<NoCopy> NoCopyFn(int i, bool success) | ||
{ | ||
if (success) return {i}; | ||
return util::Error{Untranslated(strprintf("nocopy %i error.", i))}; | ||
} | ||
|
||
template <typename T> | ||
void ExpectResult(const util::Result<T>& result, bool success, const bilingual_str& str) | ||
{ | ||
BOOST_CHECK_EQUAL(bool(result), success); | ||
BOOST_CHECK_EQUAL(util::ErrorString(result), str); | ||
} | ||
|
||
template <typename T, typename... Args> | ||
void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args) | ||
{ | ||
ExpectResult(result, true, str); | ||
BOOST_CHECK_EQUAL(result.has_value(), true); | ||
BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...}); | ||
BOOST_CHECK_EQUAL(&result.value(), &*result); | ||
} | ||
|
||
template <typename T, typename... Args> | ||
void ExpectFail(const util::Result<T>& result, const bilingual_str& str) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
ExpectResult(result, false, str); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(check_returned) | ||
{ | ||
ExpectSuccess(IntFn(5, true), {}, 5); | ||
ExpectFail(IntFn(5, false), Untranslated("int 5 error.")); | ||
ExpectSuccess(NoCopyFn(5, true), {}, 5); | ||
ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error.")); | ||
ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S")); | ||
ExpectFail(StrFn(Untranslated("S"), false), Untranslated("str S error.")); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(check_value_or) | ||
{ | ||
BOOST_CHECK_EQUAL(IntFn(10, true).value_or(20), 10); | ||
BOOST_CHECK_EQUAL(IntFn(10, false).value_or(20), 20); | ||
BOOST_CHECK_EQUAL(NoCopyFn(10, true).value_or(20), 10); | ||
BOOST_CHECK_EQUAL(NoCopyFn(10, false).value_or(20), 20); | ||
BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), Untranslated("A")); | ||
BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B")); | ||
} | ||
|
||
BOOST_AUTO_TEST_SUITE_END() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,45 +5,80 @@ | |
#ifndef BITCOIN_UTIL_RESULT_H | ||
#define BITCOIN_UTIL_RESULT_H | ||
|
||
#include <attributes.h> | ||
#include <util/translation.h> | ||
|
||
#include <variant> | ||
|
||
/* | ||
* 'BResult' is a generic class useful for wrapping a return object | ||
* (in case of success) or propagating the error cause. | ||
*/ | ||
template<class T> | ||
class BResult { | ||
namespace util { | ||
|
||
struct Error { | ||
bilingual_str message; | ||
}; | ||
|
||
//! The util::Result class provides a standard way for functions to return | ||
//! either error messages or result values. | ||
//! | ||
//! It is intended for high-level functions that need to report error strings to | ||
//! end users. Lower-level functions that don't need this error-reporting and | ||
//! only need error-handling should avoid util::Result and instead use standard | ||
//! classes like std::optional, std::variant, and std::tuple, or custom structs | ||
//! and enum types to return function results. | ||
//! | ||
//! Usage examples can be found in \example ../test/result_tests.cpp, but in | ||
//! general code returning `util::Result<T>` values is very similar to code | ||
//! returning `std::optional<T>` values. Existing functions returning | ||
//! `std::optional<T>` can be updated to return `util::Result<T>` and return | ||
//! error strings usually just replacing `return std::nullopt;` with `return | ||
//! util::Error{error_string};`. | ||
template <class T> | ||
class Result | ||
{ | ||
private: | ||
std::variant<bilingual_str, T> m_variant; | ||
|
||
public: | ||
BResult() : m_variant{Untranslated("")} {} | ||
BResult(T obj) : m_variant{std::move(obj)} {} | ||
BResult(bilingual_str error) : m_variant{std::move(error)} {} | ||
template <typename FT> | ||
ryanofsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
friend bilingual_str ErrorString(const Result<FT>& result); | ||
|
||
/* Whether the function succeeded or not */ | ||
bool HasRes() const { return std::holds_alternative<T>(m_variant); } | ||
public: | ||
Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. styling nit: what about creating an enum for the indexes? enum { ERR=0, VAL=1 };
// then
std::in_place_index_t<VAL>{} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #25721 (comment)
#25665 should drop the std::variant entirely making this moot, but using enum for this seems more indirect and fragile since nothing keeps enum and variant declarations lined up. I'm not against adding this, but would prefer not to unless there is more demand |
||
Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {} | ||
|
||
/* In case of success, the result object */ | ||
const T& GetObj() const { | ||
assert(HasRes()); | ||
return std::get<T>(m_variant); | ||
//! std::optional methods, so functions returning optional<T> can change to | ||
//! return Result<T> with minimal changes to existing code, and vice versa. | ||
bool has_value() const noexcept { return m_variant.index() == 1; } | ||
const T& value() const LIFETIMEBOUND | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #25721 (comment)
Can throw bad_variant_access according to https://en.cppreference.com/w/cpp/utility/variant/get. The equivalent std::optional method can also throw https://en.cppreference.com/w/cpp/utility/optional/value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it can throw after our assert, can it? |
||
{ | ||
assert(has_value()); | ||
return std::get<1>(m_variant); | ||
} | ||
T ReleaseObj() | ||
T& value() LIFETIMEBOUND | ||
ryanofsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
assert(HasRes()); | ||
return std::move(std::get<T>(m_variant)); | ||
assert(has_value()); | ||
return std::get<1>(m_variant); | ||
} | ||
|
||
/* In case of failure, the error cause */ | ||
const bilingual_str& GetError() const { | ||
assert(!HasRes()); | ||
return std::get<bilingual_str>(m_variant); | ||
template <class U> | ||
T value_or(U&& default_value) const& | ||
{ | ||
return has_value() ? value() : std::forward<U>(default_value); | ||
} | ||
|
||
explicit operator bool() const { return HasRes(); } | ||
template <class U> | ||
T value_or(U&& default_value) && | ||
{ | ||
return has_value() ? std::move(value()) : std::forward<U>(default_value); | ||
} | ||
explicit operator bool() const noexcept { return has_value(); } | ||
const T* operator->() const LIFETIMEBOUND { return &value(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #25721 (comment)
I guess this is a difference between std::optional and std::variant. It doesn't seem like there is a way to get the address of an object inside a variant that is noexcept. it would be possible to make this noexcept in followup #25665 which removes the std::variant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be possible, given our |
||
const T& operator*() const LIFETIMEBOUND { return value(); } | ||
T* operator->() LIFETIMEBOUND { return &value(); } | ||
T& operator*() LIFETIMEBOUND { return value(); } | ||
}; | ||
|
||
template <typename T> | ||
bilingual_str ErrorString(const Result<T>& result) | ||
{ | ||
return result ? bilingual_str{} : std::get<0>(result.m_variant); | ||
} | ||
} // namespace util | ||
|
||
#endif // BITCOIN_UTIL_RESULT_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add
util/check.h
include header forAssert