Skip to content

Commit

Permalink
refactor: Add util::Result multiple error and warning messages
Browse files Browse the repository at this point in the history
Add util::Result support for returning warning messages and multiple errors,
not just a single error string. This provides a way for functions to report
errors and warnings in a standard way, and simplifies interfaces.

The functionality is unit tested here, and put to use in followup PR
bitcoin#25722
  • Loading branch information
ryanofsky committed Dec 6, 2024
1 parent 2dd4849 commit 643b881
Show file tree
Hide file tree
Showing 5 changed files with 244 additions and 22 deletions.
1 change: 1 addition & 0 deletions src/kernel/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ add_library(bitcoinkernel
../util/hasher.cpp
../util/moneystr.cpp
../util/rbf.cpp
../util/result.cpp
../util/serfloat.cpp
../util/signalinterrupt.cpp
../util/strencodings.cpp
Expand Down
74 changes: 71 additions & 3 deletions src/test/result_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <tinyformat.h>
#include <util/check.h>
#include <util/result.h>
#include <util/translation.h>

#include <algorithm>
#include <boost/test/unit_test.hpp>
#include <memory>
#include <ostream>
#include <string>
#include <utility>

inline bool operator==(const bilingual_str& a, const bilingual_str& b)
{
Expand Down Expand Up @@ -90,15 +97,60 @@ enum FnError { ERR1, ERR2 };

util::Result<int, FnError> IntFailFn(int i, bool success)
{
if (success) return i;
if (success) return {util::Warning{Untranslated(strprintf("int %i warn.", i))}, i};
return {util::Error{Untranslated(strprintf("int %i error.", i))}, i % 2 ? ERR1 : ERR2};
}

util::Result<std::string, FnError> StrFailFn(int i, bool success)
{
util::Result<std::string, FnError> result;
if (auto int_result{IntFailFn(i, success) >> result}) {
result.Update(strprintf("%i", *int_result));
} else {
result.Update({util::Error{Untranslated("str error")}, int_result.GetFailure()});
}
return result;
}

util::Result<NoCopyNoMove, FnError> EnumFailFn(FnError ret)
{
return {util::Error{Untranslated("enum fail.")}, ret};
}

util::Result<void> WarnFn()
{
return {util::Warning{Untranslated("warn.")}};
}

util::Result<int> MultiWarnFn(int ret)
{
util::Result<int> result;
for (int i = 0; i < ret; ++i) {
result.AddWarning(Untranslated(strprintf("warn %i.", i)));
}
result.Update(ret);
return result;
}

util::Result<void, int> ChainedFailFn(FnError arg, int ret)
{
util::Result<void, int> result{util::Error{Untranslated("chained fail.")}, ret};
EnumFailFn(arg) >> result;
WarnFn() >> result;
return result;
}

util::Result<int, FnError> AccumulateFn(bool success)
{
util::Result<int, FnError> result;
util::Result<int> x = MultiWarnFn(1) >> result;
BOOST_REQUIRE(x);
util::Result<int> y = MultiWarnFn(2) >> result;
BOOST_REQUIRE(y);
result.Update(IntFailFn(*x + *y, success));
return result;
}

util::Result<int, int> TruthyFalsyFn(int i, bool success)
{
if (success) return i;
Expand Down Expand Up @@ -140,7 +192,13 @@ BOOST_AUTO_TEST_CASE(check_returned)
ExpectFail(NoCopyNoMoveFn(5, false), Untranslated("nocopynomove 5 error."), 5);
ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
ExpectResult(StrFn(Untranslated("S"), false), false, Untranslated("str S error."));
ExpectSuccess(StrFailFn(1, true), Untranslated("int 1 warn."), "1");
ExpectFail(StrFailFn(2, false), Untranslated("int 2 error. str error"), ERR2);
ExpectFail(EnumFailFn(ERR2), Untranslated("enum fail."), ERR2);
ExpectFail(ChainedFailFn(ERR1, 5), Untranslated("chained fail. enum fail. warn."), 5);
ExpectSuccess(MultiWarnFn(3), Untranslated("warn 0. warn 1. warn 2."), 3);
ExpectSuccess(AccumulateFn(true), Untranslated("warn 0. warn 0. warn 1. int 3 warn."), 3);
ExpectFail(AccumulateFn(false), Untranslated("int 3 error. warn 0. warn 0. warn 1."), ERR1);
ExpectSuccess(TruthyFalsyFn(0, true), {}, 0);
ExpectFail(TruthyFalsyFn(0, false), Untranslated("failure value 0."), 0);
ExpectSuccess(TruthyFalsyFn(1, true), {}, 1);
Expand All @@ -156,15 +214,15 @@ BOOST_AUTO_TEST_CASE(check_update)
result.Update({util::Error{Untranslated("error")}, ERR1});
ExpectFail(result, Untranslated("error"), ERR1);
result.Update(2);
ExpectSuccess(result, Untranslated(""), 2);
ExpectSuccess(result, Untranslated("error"), 2);

// Test the same thing but with non-copyable success and failure types.
util::Result<NoCopy, NoCopy> result2{0};
ExpectSuccess(result2, {}, 0);
result2.Update({util::Error{Untranslated("error")}, 3});
ExpectFail(result2, Untranslated("error"), 3);
result2.Update(4);
ExpectSuccess(result2, Untranslated(""), 4);
ExpectSuccess(result2, Untranslated("error"), 4);
}

BOOST_AUTO_TEST_CASE(check_dereference_operators)
Expand All @@ -188,6 +246,16 @@ BOOST_AUTO_TEST_CASE(check_value_or)
BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B"));
}

BOOST_AUTO_TEST_CASE(check_message_accessors)
{
util::Result<void> result{util::Error{Untranslated("Error.")}, util::Warning{Untranslated("Warning.")}};
BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors.size(), 1);
BOOST_CHECK_EQUAL(Assert(result.GetMessages())->errors[0], Untranslated("Error."));
BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings.size(), 1);
BOOST_CHECK_EQUAL(Assert(result.GetMessages())->warnings[0], Untranslated("Warning."));
BOOST_CHECK_EQUAL(util::ErrorString(result), Untranslated("Error. Warning."));
}

struct Derived : NoCopyNoMove {
using NoCopyNoMove::NoCopyNoMove;
};
Expand Down
1 change: 1 addition & 0 deletions src/util/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
moneystr.cpp
rbf.cpp
readwritefile.cpp
result.cpp
serfloat.cpp
signalinterrupt.cpp
sock.cpp
Expand Down
33 changes: 33 additions & 0 deletions src/util/result.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (c) 2024 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://www.opensource.org/licenses/mit-license.php.

#include <util/result.h>

#include <algorithm>
#include <initializer_list>
#include <iterator>
#include <util/translation.h>

namespace util {
namespace detail {
bilingual_str JoinMessages(const Messages& messages)
{
bilingual_str result;
for (const auto& messages : {messages.errors, messages.warnings}) {
for (const auto& message : messages) {
if (!result.empty()) result += Untranslated(" ");
result += message;
}
}
return result;
}
} // namespace detail

void ResultTraits<Messages>::Update(Messages& dst, Messages& src) {
dst.errors.insert(dst.errors.end(), std::make_move_iterator(src.errors.begin()), std::make_move_iterator(src.errors.end()));
dst.warnings.insert(dst.warnings.end(), std::make_move_iterator(src.warnings.begin()), std::make_move_iterator(src.warnings.end()));
src.errors.clear();
src.warnings.clear();
}
} // namespace util
Loading

0 comments on commit 643b881

Please sign in to comment.