refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

ryanofsky · 2022-09-01T19:47:18Z

Replace uses of std::optional<bilingual_str> with util::Result as suggested #25648 (comment), #27632 (comment), #27632 (comment), #24313 (comment)

DrahtBot · 2022-09-01T21:26:13Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, TheCharlatan, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27300 (build: debug enable addrman consistency checks by willcl-ark)
#26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky · 2022-09-12T18:53:00Z

A minimal (but hacky) way to add support for void to Result originally posted bitcoin#27632 (comment)

maflcko · 2023-05-24T14:29:23Z

Looks like there is a clang bug. Could be fixed by either bumping it again (to clang-13), see #27682, but I am not sure how to do that for macOS. Or by replacing return addrman; with return {std::move(addrman)};, or something like that (temporarily).

godbolt for clang-12 (broken): https://godbolt.org/z/1hraPjxrf

ryanofsky · 2023-05-24T19:35:17Z

Thanks, added std::move as suggested.

Updated b2bcba0 -> 8aa8f73 (pr/bresult-opt.10 -> pr/bresult-opt.11, compare) with workaround for CI errors https://cirrus-ci.com/task/6456067106799616?logs=ci#L1528 https://cirrus-ci.com/task/5048692223246336?logs=ci#L1240 https://cirrus-ci.com/task/6174592130088960?logs=ci#L1328

addrdb.cpp:213:12: error: call to implicitly-deleted copy constructor of 'util::Result<std::__1::unique_ptr<AddrMan, std::__1::default_delete<AddrMan> > >::T' (aka 'std::__1::unique_ptr<AddrMan, std::__1::default_delete<AddrMan> >')
    return addrman;
           ^~~~~~~
/usr/lib/llvm-10/bin/../include/c++/v1/memory:2513:3: note: copy constructor is implicitly deleted because 'unique_ptr<AddrMan, std::__1::default_delete<AddrMan> >' has a user-declared move constructor
  unique_ptr(unique_ptr&& __u) _NOEXCEPT
  ^
./util/result.h:47:14: note: passing argument to parameter 'obj' here
    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
             ^
1 error generated.

maflcko · 2023-05-25T07:17:31Z

very nice ACK 8aa8f73 🏏

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏
FlnWlOVGKZVAPgirGB1Me+n1IHxeaKXXl2LHKPIpsnt+N2jG0ILNCy/guKB9V+K/JzCbDLy9h/UJBgDsXbspAw==

maflcko · 2023-05-25T07:57:35Z

src/txdb.h

@@ -98,6 +99,6 @@ class CBlockTreeDB : public CDBWrapper
        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 };

-std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);
+util::Result<void> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);


nit (if you retouch): Could add [[nodiscard]] (either as fixup or new commit), while touching all those header files?

TheCharlatan

ACK 8aa8f73

I think [[nodiscard] qualifiers for all the util::Result return types would be nice.

hebasto

ACK 8aa8f73, I have reviewed the code and it looks OK.

hebasto · 2023-05-26T10:01:37Z

src/util/result.h

    std::variant<bilingual_str, T> m_variant;

    template <typename FT>
    friend bilingual_str ErrorString(const Result<FT>& result);

 public:
+    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void


5f49cb1

nit: Add #include <utility>?

hebasto · 2023-05-26T12:54:02Z

Looks like there is a clang bug. Could be fixed by either bumping it again, see #27682, but I am not sure how to do that for macOS. Or by replacing return addrman; with return {std::move(addrman)};, or something like that (temporarily).

Although GCC complains:

$ ./configure CONFIG_SITE=$PWD/depends/arm-linux-gnueabihf/share/config.site --enable-suppress-external-warnings CXXFLAGS="-Wextra -Wno-psabi"
$ make > /dev/null 
addrdb.cpp: In function ‘util::Result<std::unique_ptr<AddrMan> > LoadAddrman(const NetGroupManager&, const ArgsManager&)’:
addrdb.cpp:213:21: warning: redundant move in return statement [-Wredundant-move]
  213 |     return std::move(addrman); // std::move should be unneccessary but is temporarily needed to work around clang bug (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092)
      |            ~~~~~~~~~^~~~~~~~~
addrdb.cpp:213:21: note: remove ‘std::move’ call

… with `util::Result` 8aa8f73 refactor: Replace std::optional<bilingual_str> with util::Result (Ryan Ofsky) 5f49cb1 util: Add void support to util::Result (MarcoFalke) Pull request description: Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested bitcoin#25648 (comment), bitcoin#27632 (comment), bitcoin#27632 (comment), bitcoin#24313 (comment) ACKs for top commit: MarcoFalke: very nice ACK 8aa8f73 🏏 TheCharlatan: ACK 8aa8f73 hebasto: ACK 8aa8f73, I have reviewed the code and it looks OK. Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30

maflcko · 2023-05-29T08:00:17Z

Yeah, that's why I suggested {std::move()}, not std::move(), see #25977 (comment)

…ng a Result return type is an error fa5680b fix includes for touched header files (iwyu) (MarcoFalke) dddde27 Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke) Pull request description: Only add it for those where it is an error to ignore. Also, fix the gcc compile warning bitcoin/bitcoin#25977 (comment). Also, fix iwyu for touched header files. ACKs for top commit: TheCharlatan: ACK fa5680b stickies-v: ACK fa5680b Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415

…ult return type is an error fa5680b fix includes for touched header files (iwyu) (MarcoFalke) dddde27 Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke) Pull request description: Only add it for those where it is an error to ignore. Also, fix the gcc compile warning bitcoin#25977 (comment). Also, fix iwyu for touched header files. ACKs for top commit: TheCharlatan: ACK fa5680b stickies-v: ACK fa5680b Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415

Summary: > Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason. > > This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the > return object and another ref for the error string. We can simply return a 'BResult<Obj>'. > > Example of what we currently have: > ``` > bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) { > do something... > if (error) { > error_string = "something bad happened"; > return false; > } > > result = goodResult; > return true; > } > ``` > > Example of what we will get with this commit: > ``` > BResult<Obj> doSomething(arg1, arg2, arg3, arg4) { > do something... > if (error) return {"something happened"}; > > // good > return {goodResult}; > } > ``` > > This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra > pre-function-call error string and result object declarations to pass the references to the function. bitcoin/bitcoin@7a45c33 ---- > Prepare BResult for non-copyable types bitcoin/bitcoin@fa8de09 ---- > Refactor: Replace BResult with util::Result > > Rename `BResult` class to `util::Result` and update the class interface to be > more compatible with `std::optional` and with a full-featured result class > implemented in bitcoin/bitcoin#25665. Motivation for > this change is to update existing `BResult` usages now so they don't have to > change later when more features are added in #25665. > > This change makes the following improvements originally implemented in #25665: > > - More explicit API. Drops potentially misleading `BResult` constructor that > treats any bilingual string argument as an error. Adds `util::Error` > constructor so it is never ambiguous when a result is being assigned an error > or non-error value. > > - Better type compatibility. Supports `util::Result<bilingual_str>` return > values to hold translated messages which are not errors. > > - More standard and consistent API. `util::Result` supports most of the same > operators and methods as `std::optional`. `BResult` had a less familiar > interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj > naming was also not internally consistent. > > - Better code organization. Puts `src/util/` code in the `util::` namespace so > naming reflects code organization and it is obvious where the class is coming > from. Drops "B" from name because it is undocumented what it stands for > (bilingual?) > > - Has unit tests. bitcoin/bitcoin@a23cca5 ---- > util: Add void support to util::Result > > A minimal (but hacky) way to add support for void to Result > originally posted bitcoin/bitcoin#27632 (comment) bitcoin/bitcoin@5f49cb1 ---- This is a partial backport of [[bitcoin/bitcoin#25218 | core#25218]], [[bitcoin/bitcoin#25594 | core#25594]], [[bitcoin/bitcoin#25721 | core#25721]] and [[bitcoin/bitcoin#25977 | core#25977]] The wallet use cases in the first 3 pull requests are not yet applicable to Bitcoin ABC due to missing dependencies. The `std::optional<bilingual_str>` use cases from [[bitcoin/bitcoin#25977 | core#25977]] will be backported in their own diff. Test Plan: `ninja && ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14991

Summary: > refactor: Replace std::optional<bilingual_str> with util::Result bitcoin/bitcoin@8aa8f73 > Add [[nodiscard]] where ignoring a Result return type is an error bitcoin/bitcoin@dddde27 > fix includes for touched header files (iwyu) bitcoin/bitcoin@fa5680b This is a partial backport of [[bitcoin/bitcoin#25977 | core#25977]] and [[bitcoin/bitcoin#27774 | core#27774]] Test Plan: `ninja all check-all` `grep -r 'std::optional<bilingual_str>' src/` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D14992

DrahtBot added the Refactoring label Sep 1, 2022

ryanofsky force-pushed the pr/bresult-opt branch from 9d3918d to ebc493e Compare September 12, 2022 18:51

DrahtBot added the Needs rebase label Sep 13, 2022

DrahtBot mentioned this pull request Sep 14, 2022

Multiprocess bitcoin #10102

Draft

ryanofsky force-pushed the pr/bresult-opt branch from ebc493e to f1a9c71 Compare September 20, 2022 17:39

DrahtBot removed the Needs rebase label Sep 20, 2022

DrahtBot mentioned this pull request Sep 21, 2022

test: clean and extend availablecoins_tests coverage #25789

Merged

DrahtBot added the Needs rebase label Oct 13, 2022

ryanofsky force-pushed the pr/bresult-opt branch from f1a9c71 to fb94363 Compare October 14, 2022 21:56

DrahtBot added Needs rebase and removed Needs rebase labels Oct 14, 2022

ryanofsky force-pushed the pr/bresult-opt branch from fb94363 to 080268d Compare January 20, 2023 23:24

DrahtBot removed the Needs rebase label Jan 21, 2023

ryanofsky mentioned this pull request May 12, 2023

Raise on invalid -debug and -loglevel config options #27632

Merged

ryanofsky mentioned this pull request May 24, 2023

Improve display address handling for external signer #24313

Merged

MarcoFalke and others added 2 commits May 24, 2023 08:55

util: Add void support to util::Result

5f49cb1

A minimal (but hacky) way to add support for void to Result originally posted bitcoin#27632 (comment)

refactor: Replace std::optional<bilingual_str> with util::Result

8aa8f73

ryanofsky force-pushed the pr/bresult-opt branch from 74655e5 to b2bcba0 Compare May 24, 2023 13:43

ryanofsky marked this pull request as ready for review May 24, 2023 13:45

DrahtBot added CI failed and removed Needs rebase labels May 24, 2023

ryanofsky force-pushed the pr/bresult-opt branch from b2bcba0 to 8aa8f73 Compare May 24, 2023 19:32

This was referenced May 25, 2023

build: debug enable addrman consistency checks #27300

Closed

bugfix: Make CCheckQueue RAII-styled (attempt 2) #26762

Merged

DrahtBot removed the CI failed label May 25, 2023

maflcko requested a review from TheCharlatan May 25, 2023 07:18

maflcko reviewed May 25, 2023

View reviewed changes

TheCharlatan approved these changes May 25, 2023

View reviewed changes

maflcko assigned ryanofsky May 25, 2023

hebasto approved these changes May 26, 2023

View reviewed changes

fanquake merged commit 4d13fe4 into bitcoin:master May 26, 2023

maflcko mentioned this pull request May 29, 2023

refactor: Add [[nodiscard]] where ignoring a Result return type is an error #27774

Merged

bitcoin locked and limited conversation to collaborators May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

Uh oh!

ryanofsky commented Sep 1, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Sep 1, 2022 •

edited

Loading

Uh oh!

ryanofsky commented Sep 12, 2022 •

edited

Loading

Uh oh!

maflcko commented May 24, 2023 •

edited

Loading

Uh oh!

ryanofsky commented May 24, 2023

Uh oh!

maflcko commented May 25, 2023

Uh oh!

maflcko May 25, 2023

Uh oh!

TheCharlatan left a comment

Uh oh!

hebasto left a comment

Uh oh!

hebasto May 26, 2023

Uh oh!

hebasto commented May 26, 2023

Uh oh!

maflcko commented May 29, 2023

Uh oh!

Uh oh!

refactor: Replace std::optional<bilingual_str> with util::Result #25977

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Uh oh!

Conversation

ryanofsky commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented May 24, 2023

Uh oh!

maflcko commented May 25, 2023

Uh oh!

maflcko May 25, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto May 26, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 26, 2023

Uh oh!

maflcko commented May 29, 2023

Uh oh!

Uh oh!

refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

ryanofsky commented Sep 1, 2022 •

edited

Loading

DrahtBot commented Sep 1, 2022 •

edited

Loading

ryanofsky commented Sep 12, 2022 •

edited

Loading

maflcko commented May 24, 2023 •

edited

Loading