The PR branch HEAD was 590bc615a at the time of this review club meeting.
Notes
This PR is a followup to #25218 introducing helper class BResult, which we discussed in a previous review club.
In #25721BResult 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.
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()?
#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?
What is a union? Why is m_value a union type when it holds just one member?
<stickies-v> welcome everyone! Today we're looking at #25665, authored by ryanofsky. The notes and questions are available on https://bitcoincore.reviews/25665
<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.
<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.
<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
<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." (https://en.cppreference.com/w/cpp/utility/optional)
<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
<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").
<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
<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.
<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()`?
<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`)
<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
<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.
<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?
<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).
<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
<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?
<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)
<stickies-v>https://github.com/bitcoin/bitcoin/pull/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?
<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)
<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?
<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?
<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)
<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
<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.
<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.
<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!
<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
<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>`?
<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)
<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?