Add util::Result failure values, multiple error and warning messages (refactoring)

Host: stickies-v  -  PR author: ryanofsky

The PR branch HEAD was 590bc615a at the time of this review club meeting.


  • This PR is a followup to #25218 introducing helper class BResult, which we discussed in a previous review club.

  • In #25721 BResult was renamed to util::Result, and the interface modified to resemble that of std::optional.

  • The description of this latest PR #25665 comprehensively lists the introduced changes, as well as useful information on history and alternatives. In summary, the proposed functional changes compared to the initial BResult design are to:
    • allow a result to have both a value and an error message, instead of just one of both.
    • allow a result to store multiple errors and warnings, instead of just a single error.
    • allow multiple results to be chained, where a util::Result can be instantiated from another util::Result and take over its errors and warnings.
  • This PR is quite heavy on generic programming and templating. Since util::Result is a helper class, we want it to be usable in a wide range of use cases, and can’t make assumptions about the types of values it will hold.

  • The util::Result class draws parallels with the Rust std::result Result type.


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. Which use cases previously not handled well by util::Result does this PR target? Why did util::Result need an upgrade to be compatible with LoadChainState() and VerifyLoadedChainState()?

  3. #25721 states BResult had a less familiar interface than the well-known std::optional. Is that a good enough reason to change it? What other reason(s) do you see for this change?

  4. What is a union? Why is m_value a union type when it holds just one member?

  5. In template <typename T, typename F> class ResultBase, what do T and F represent conceptually?

  6. Why does #25665 implement custom Construct methods instead of just using the regular constructor methods?

  7. For which type(s) T is util::detail::MoveElements(T& src, T& dest) instantiated? Do you think a templated approach makes sense here?

  8. What is the purpose of having differents structs Warning and Error when they both just wrap a bilingual_str. Could we just use an alias instead?

Meeting Log

  117:00 <stickies-v> #startmeeting
  217:00 <pablomartin_> hi all
  317:00 <larryruane_> hi
  417:00 <brunoerg> hi
  517:00 <michaelfolkson> hi
  617:00 <aryan_> πŸ‘‹
  717:01 <Amirreza> Hi
  817:01 <stickies-v> welcome everyone! Today we're looking at #25665, authored by ryanofsky. The notes and questions are available on
  917:01 <schmidty_> hi
 1017:01 <juancama> Hey everyone
 1117:02 <stickies-v> anyone joining us for the first time today? even if you're just lurking, feel free to say hi!
 1217:02 <furszy> hi
 1317:02 <bot_21> hi
 1417:02 <hernanmarino> Hi !
 1517:02 <aryan_> First timer here.
 1617:02 <effexzi> Hi every1
 1717:03 <stickies-v> whoo, welcome aryan_ ! glad to have you here. don't hold back on asking questions if anything's unclear!
 1817:05 <pablomartin> welcome aryan_!
 1917:05 <stickies-v> today's meeting is fairly technical and focuses on a couple of c++ libraries/techniques that aren't very often used in the codebase. as a quick disclaimer, i'm fairly new to c++ myself so don't take anything I say as gospel either haha.
 2017:05 <stickies-v> who got the chance to review the PR or read the notes? (y/n)
 2117:05 <pablomartin> y
 2217:05 <hernanmarino> y
 2317:05 <aryan_> y
 2417:05 <Lov3r_Of_Bitcoin> y
 2517:05 <Amirreza> y
 2617:05 <michaelfolkson> y
 2717:05 <brunoerg> y
 2817:05 <juancama> y
 2917:06 <larryruane_> y
 3017:06 <bot_21> n
 3117:06 <stickies-v> what a y streak, oh my! and great to see some of you having commented on the PR already as well
 3217:06 <stickies-v> for those of you who were able to review, would you give it a Concept ACK, Approach ACK, Tested ACK, or NACK?
 3317:06 <sipa> n
 3417:06 <aryan_> Approach ACK
 3517:06 <Amirreza> Tested Ack
 3617:06 <brunoerg> Concept ACK
 3717:07 <larryruane_> I have to say, though, this is the most confused I've ever been for a review club. This is well beyond my c++ understanding
 3817:07 <pablomartin> Concept ACK and tested ACK
 3917:07 <hernanmarino> Tested ACK, but have some doubts on some technical details of the implementation
 4017:07 <stickies-v> larryruane_: glad to hear I wasn't alone in that
 4117:08 <stickies-v> hernanmarino: care to share what they are? or will we cover them in the questions?
 4217:08 <brunoerg> larryruane_: +1
 4317:08 <aryan_> Ditto. This is the first C++ I've looked at in ~8 years though so I guess to be expected.
 4417:08 <michaelfolkson> Yeah Concept ACK, a little hazy on moving from BResult to this though
 4517:09 <hernanmarino> I just don know the answer for a couple of questions, let's talk later :)
 4617:09 <aryan_> If we're free to share doubts or uncertainties, is it typical to override the `bool` operator to allow for success checks in this way? Is it typical to override the bool operator to allow to check for success/failures in this way?
 4717:09 <stickies-v> quick note before diving into the questions: the PR has been updated slightly since these notes were released, so the current commit hashes are different but the actual changes are limited to comments. I'll keep linking to the "old" (590bc61) commits here.
 4817:09 <aryan_> Please feel free to answer later, just something that hung me up.
 4917:09 <larryruane_> what I desperately need to do is go through `result_tests.cpp` and really understand the test cases, simplest first, and maybe even write some test cases of my own (like when you're learning a new language), or try varying the existing test cases
 5017:10 <stickies-v> aryan_: iirc that's to keep parity with how std::optional is implemented, but would need to double check
 5117:11 <stickies-v> yep: "When an object of type optional<T> is contextually converted to bool, the conversion returns true if the object contains a value and false if it does not contain a value." (
 5217:11 <stickies-v> (your link didn't actually point me to a diff so lmk if that's not an answer to your question)
 5317:11 <sipa> Is it a "operator bool()" or an "explicit operator bool()" ?
 5417:12 <aryan_> But this one here is true if `m_info` (or `m_info->failure) is empty. This means that it'll return false if there's a value but no `m_info`.
 5517:12 <stickies-v> larryruane_: yeah that's a good point, tests are very often a good starting point (besides docs) to understanding the use case/effects of a certain feature/change
 5617:13 <aryan_> //! Success check.
 5717:13 <aryan_> operator bool() const { return !m_info || !m_info->failure; }
 5817:13 <sipa> Before C++11, operator bool() existed for implicit conversion to bool, but it had lots of non-obvious ways to trigger it (e.g. you could write "obj + 3", and if obj had an operator bool, it'd evaluate to 3 or 4").
 5917:13 <sipa> Since C++11 you can use "explicit operator bool()" to avoid many such cases.
 6017:14 <pablomartin> thanks sipa
 6117:15 <stickies-v> aryan_: if returns true either there is no `ErrorInfo m_info`at all (i.e. success), or if there is an `ErrorInfo m_info` but it doesn't contain failure data. this is because the new `Result` implementation can also store warnings, which still mean the function returned successfully
 6217:16 <aryan_> Right, right. It seems to means of gauging success. Just feels odd to use the `bool` operator for that. Having a successful result with a value present return false is just something new, I guess.
 6317:16 <stickies-v> alright moving on to the first question: which use cases previously not handled well by `util::Result` does this PR target? Why did `util::Result` need an upgrade to be compatible with `LoadChainState()` and `VerifyLoadedChainState()`?
 6417:16 <sipa> operator bool() is generally a bad idea
 6517:16 <stickies-v> aryan_: we'll actually cover why that parity with std::optional is important in the next question!
 6617:17 <Amirreza> I think having multiple errors and warnings.
 6717:17 <hernanmarino> Uses cases : allow a result to have both a value and an error message,to store multiple errors and for multiple results to be chained.
 6817:17 <sipa> std::optional has "explicit operator bool()", not "operator bool()".
 6917:17 <aryan_> `Result` now allows a value AND an error message to be returned. It also allows for multiple errors/warnings. Lastly, `Result`s are now chainable (you can instantiate a `Result` with another `Result`)
 7017:18 <Amirreza> And also I think they support more types for Error value (not just string)
 7117:18 <hernanmarino> and LoadChainState() needed to return a value on Errors, not only on success, something not possible before this PR
 7217:18 <brunoerg> It also allows to return a value on failure, not just a value on success I guess
 7317:19 <brunoerg> hernanmarino: +1
 7417:19 <pablomartin> yeah, to have both value and error messages, store multiple of them, and buid them on top of them
 7517:19 <stickies-v> Amirreza: hernanmarino aryan_ brunoerg pablomartin yes that pretty much covers it all! and nice one catching the chaining bit aryan_ , since that wasn't covered in the PR description
 7617:20 <michaelfolkson> Oh and with BResult you could only have one, ok
 7717:20 <larryruane_> pablomartin: but I think not storing multiple values... only multiple error and warnings
 7817:20 <larryruane_> (maybe that's what you meant)
 7917:20 <stickies-v> michaelfolkson: yep indeed, but for example LoadChainState() returns both a status code and an (optional) error string
 8017:20 <stickies-v> single value, multiple errors/warnings indeed! thanks larryruane_
 8117:20 <pablomartin> larryuane_ yeah, i meant multiple of the previous (values and error msgs)
 8217:21 <furszy> i'm a bit hesitant here, warnings are another specialization of the error field. Not sure if it's something that should be placed on the base ErrorInfo class.
 8317:22 <pablomartin> larryuane: got you, thanks!
 8417:22 <stickies-v> so bonus question... what if we have a function that needs to return multiple values on failure? should we extend `Result` to handle that?
 8517:22 <stickies-v> furszy: I think the distinction is meaningful because warnings don't change whether the function returned successfully?
 8617:24 <Amirreza> stickies-v: Currently by using the error-vectors we can do this, am I correct? But not return multiple value with different types.
 8717:24 ← juancama6 left (
 8817:24 <aryan_> I think warnings _DO_ change whether the function returned successfully (that bool I hate so much will end up returning false if there's a warning which means it failed).
 8917:24 <larryruane_> stickies-v: I would say no because the return value can be a `std::tuple`
 9017:24 <aryan_> +1 @larryuane_
 9117:24 <stickies-v> Amirreza: we only use vectors for error and warnings *messages*, not the failure *value*
 9217:25 <Amirreza> stickies-v: Yes, thanks.
 9317:25 <larryruane_> one could say that `tuple` is less convenient than if there was built-in support, but it's probably not very common, so best to keep it simple
 9417:25 <furszy> :stickies-v meaningful distinction to what exactly? not sure if I understood the question
 9517:26 <stickies-v> larryruane_: yeah exactly, std::tuple or the likes, or even a custom struct if we need something more complex
 9617:28 <pablomartin> but then has_value would mean that could have 1 or multiple...
 9717:28 <stickies-v> well a function could return successfully, but still provide feedback on some potentially dangerous operations/contexts through the warnings? but without distinguishing between errors and warnings, that would be difficult?
 9817:28 <furszy> not every function requires that.
 9917:29 <furszy> actually, most of the current util::Result usages don't requires it
10017:29 <stickies-v> ah, no that's true
10117:29 <aryan_> It should but with warnings being stored in `m_info`, it'll cause the bool to return false (unsuccessful) even if `m_info` has no errors. (sorry if it sounds like I'm harping on this bool)
10217:31 <stickies-v> hmm no it wouldn't, aryan_ because `!m_info->failure` would be `true`
10317:31 <stickies-v> (`operator bool() const { return !m_info || !m_info->failure; }`)
10417:31 <aryan_> Ahhhh, yes, yes, yes. Sorry. I read bad.
10517:32 <stickies-v> pablomartin: not sure what you mean with 1 or multiple?
10617:32 <larryruane_> if a function accumulates some warnings... then encounters and error (or multiple errors), the warnings aren't reported too, correct?
10717:32 <stickies-v> alright gonna move on to the next question already but feel free to keep discussing previous questions - we're async!
10817:32 <stickies-v> states `BResult` had a less familiar interface than the well-known `std::optional`. Is that a good enough reason to change it? What other reason(s) do you see for this change?
10917:33 <furszy> :larryruant I think that we should always have errors > warnings
11017:33 <larryruane_> I would say it's a good enough reason, given that `BResult` is not already used by a lot of existing code (if it was, there would be an argument)
11117:33 <larryruane_> furszy: yes i agree, just checking
11217:33 <stickies-v> great question larryruane_ - to my understanding everything propagates, so I'd think warnings are kept?
11317:34 <aryan_> +1 larry
11417:35 <stickies-v> larryruane_: right, that's definitely an important argument - it would be harder case to make if we'd have to refactor tons of code. but do you see a "positive" argument as well, besides making it easier perhaps for developers because they already know the std::optional interface?
11517:36 <larryruane_> stickies-v: "What other reason(s) do you see for this change?" ... PR 25721 says "The Result/Res/Obj naming was also not internally consistent." ... maybe that's another one?
11617:36 <michaelfolkson> stickies-v: I would say....a less familiar interface on its own probably isn't a good enough reason to change it. But there are other stronger reasons (e.g. misleading BResult constructor, type compatibility from ##25721)
11717:36 <larryruane_> (but I don't understand why those are not internally consistent)
11817:37 <stickies-v> (I don't understand that internal inconsistency either haha)
11917:37 <stickies-v> michaelfolkson: I mean there are definitely more arguments in favour of this PR! was just focusing on the std::optional interface here
12017:38 <michaelfolkson> Ok sorry :)
12117:39 <stickies-v> I think a good argument in favour of the std::optional interface that a lot of the functions (supposedly?) that could benefit from a util::Result instead currently return a std::optional. so by keeping the interface identical, there should be less code change when replaced
12217:40 <larryruane_> i wonder if anyone considered
12317:41 <stickies-v> next one: what is a `union`? Why is `m_value` a `union` type when it holds just one member?
12417:42 <larryruane_> `union` because the constructors / destructors don't automatically run
12517:42 <Amirreza> I asked this question from the PR author, using union prevents calling the c-tor in the failure.
12617:42 <larryruane_> have to admit i cheated on this one
12717:42 <hernanmarino> a union is a structure for holding "one of many" value types.
12817:42 <hernanmarino> Using an union in this case avoids m_value getting constructor and destructor being called automatically, so that in case of failure m_value is never constructed.
12917:43 <Amirreza> larryruane_, I cheated too :)
13017:44 <stickies-v> larryruane_: Amirreza hernanmarino yeah exactly, feels like a sneaky workaround but does seem to do the trick!
13117:44 <aryan_> > Using an union in this case avoids m_value getting constructor and destructor being called automatically, so that in case of failure m_value is never constructed.
13217:44 <aryan_> This helps a lot in my understanding. Thank you!
13317:44 <pablomartin> yeah, like @hernanmarino - it's also on a comment in the code: Uses anonymous union so success value is never
13417:44 <pablomartin> constructed in failure case.
13517:44 <larryruane_> there's a difference between a simple `union` (which is what this is) and a `union class` right?
13617:45 <stickies-v> pablomartin: yeah well it used to not be a comment until Amirreza decided to front-run the review club haha. it's definitely something that should be in the code comments though!
13717:45 <brunoerg> `m_value` is a union to hold only success value
13817:46 <larryruane_> nevermind i guess it's always a class (I was thinking it was similar to enum versus enum class, those are different)
13917:46 <stickies-v> larryruane_: from what I understand a union is just a class type?
14017:46 <brunoerg> so it wouldn't fit with another type
14117:46 <larryruane_> stickies-v: +1
14217:47 <stickies-v> brunoerg: yup, and because `Result` doesn't always hold an m_value (in failure case), we don't want to allocate memory to it if not necessary
14317:47 <brunoerg> interesting
14417:47 <furszy> well, in that case, why m_value is an union, while the error member don't?
14517:48 <furszy> does a Result always holds an error?
14617:50 <stickies-v> furszy: that's an excellent question, and I believe `ErrorInfo` achieves something similar by dynamically checking the type of F with `std::is_same<F, void>`?
14717:50 <larryruane_> a `union` is similar to (but more basic than) a `std::variant` but the `variant` type keeps track of which variant an instance currently contains (whereas a `union` doesn't, that's up to you)
14817:50 <stickies-v> hmm no that's not true
14917:50 <sipa> std::is_same is a compile-time check, not a runtime one
15017:50 <stickies-v> (my previous statement)
15117:50 <stickies-v> yeah sorry
15217:53 <stickies-v> second attempt: `m_info` is a std::unique_pointer and we allocate memory dynamically:
15317:54 <stickies-v> but we'd still allocate memory for the failure type even if the ErrorInfo only contained errors and warnings I think? which perhaps could be improved?
15417:54 <stickies-v> furszy: do you think that makes sense?
15517:55 <stickies-v> moving on to the next one already: in `template <typename T, typename F> class ResultBase`, what do `T` and `F` represent conceptually?
15617:55 <michaelfolkson> stickies-v: Confused, errors are associated with failures?
15717:55 <stickies-v> (link:
15817:56 <furszy> > but we'd still allocate memory for the failure type even if the ErrorInfo only contained errors and warnings I think?
15917:57 <larryruane_> `T` is the type of success return value, and `F` is the type for failure return values?
16017:57 <furszy> hmm, we will always allocate memory for the error reason if it contains errors and warnings
16117:57 <Amirreza> +1 larryruane_
16217:59 <stickies-v> michaelfolkson: yeah, see the difference in constructors when Warning and Error is passed, Error triggers InitFailure, Warning doesn't:
16318:00 <hernanmarino> I agree with larryruane_
16418:00 <stickies-v> yes you're correct larryruane_ ! and everyone agreeing with him whoo
16518:01 <stickies-v> alright that's a wrap for today, thanks everyone for showing up and participating!
16618:01 <stickies-v> #endmeeting