refactor: Replace `std::optional<bilingual

ryanofsky commented at 7:47 pm on September 1, 2022: contributor

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

DrahtBot added the label Refactoring on Sep 1, 2022

DrahtBot commented at 9:26 pm on September 1, 2022: contributor

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 force-pushed on Sep 12, 2022

ryanofsky commented at 6:53 pm on September 12, 2022: contributor

Updated 9d3918d47daf7b118507ea6db0dcef69cedf7005 -> ebc493e94bbc43b6611b7211b77c12c453171ce9 (pr/bresult-opt.1 -> pr/bresult-opt.2, compare) to fix CI compile error https://cirrus-ci.com/task/6195420723412992?logs=ci#L2419 Rebased ebc493e94bbc43b6611b7211b77c12c453171ce9 -> f1a9c713540b3f00cea3c29ace4958b77390a8ef (pr/bresult-opt.2 -> pr/bresult-opt.3, compare) on top of pr/bresult2.24 to fix conflicts in base PR #25665 Rebased f1a9c713540b3f00cea3c29ace4958b77390a8ef -> fb94363ba45acd0b5ca702a60f41533a7b3fbec4 (pr/bresult-opt.3 -> pr/bresult-opt.4, compare) due to conflict with #25667 Rebased fb94363ba45acd0b5ca702a60f41533a7b3fbec4 -> 080268dc424301198ae93c3d002c13dfbfe6b9c9 (pr/bresult-opt.4 -> pr/bresult-opt.5, compare) due to conflict with #26251 and #26286 Rebased 080268dc424301198ae93c3d002c13dfbfe6b9c9 -> 2804dd4033f9ca636ff7df74453b9ddc048417de (pr/bresult-opt.5 -> pr/bresult-opt.6, compare) Rebased 2804dd4033f9ca636ff7df74453b9ddc048417de -> 4883d30d6a764d9fa551ceecdc807fa3283c891a (pr/bresult-opt.6 -> pr/bresult-opt.7, compare) on top of conflicted pr/bresult2.30, Rebased 4883d30d6a764d9fa551ceecdc807fa3283c891a -> 59c96ae3342d1e396a1df63d0af8457ecf60964a (pr/bresult-opt.7 -> pr/bresult-opt.8, compare) due to conflict with #27254 Rebased 59c96ae3342d1e396a1df63d0af8457ecf60964a -> 74655e5870888e0165c62fec75000fe04f062147 (pr/bresult-opt.8 -> pr/bresult-opt.9, compare) on top of pr/bresult2.33 #27254 Rebased 74655e5870888e0165c62fec75000fe04f062147 -> b2bcba068e2c5542ab157e62c82bb868f678beeb (pr/bresult-opt.9 -> pr/bresult-opt.10, compare) dropping dependency on #27254

DrahtBot added the label Needs rebase on Sep 13, 2022

ryanofsky force-pushed on Sep 20, 2022

DrahtBot removed the label Needs rebase on Sep 20, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

ryanofsky force-pushed on Oct 14, 2022

DrahtBot removed the label Needs rebase on Oct 14, 2022

DrahtBot added the label Needs rebase on Oct 19, 2022

ryanofsky force-pushed on Jan 20, 2023

DrahtBot removed the label Needs rebase on Jan 21, 2023

DrahtBot added the label Needs rebase on Jan 27, 2023

ryanofsky force-pushed on Feb 11, 2023

DrahtBot removed the label Needs rebase on Feb 11, 2023

DrahtBot added the label Needs rebase on Feb 22, 2023

ryanofsky force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 2, 2023

DrahtBot added the label Needs rebase on Mar 8, 2023

ryanofsky force-pushed on Apr 7, 2023

DrahtBot removed the label Needs rebase on Apr 7, 2023

DrahtBot added the label CI failed on Apr 23, 2023

ryanofsky force-pushed on May 3, 2023

DrahtBot removed the label CI failed on May 3, 2023

DrahtBot added the label Needs rebase on May 9, 2023

util: Add void support to util::Result

A minimal (but hacky) way to add support for void to Result
originally posted https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1195604095

5f49cb1bc8

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

ryanofsky force-pushed on May 24, 2023

ryanofsky marked this as ready for review on May 24, 2023

maflcko commented at 2:29 pm on May 24, 2023: member

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

DrahtBot added the label CI failed on May 24, 2023

DrahtBot removed the label Needs rebase on May 24, 2023

ryanofsky force-pushed on May 24, 2023

ryanofsky commented at 7:35 pm on May 24, 2023: contributor

Thanks, added std::move as suggested.

Updated b2bcba068e2c5542ab157e62c82bb868f678beeb -> 8aa8f73adce72e1ae855b43413c1f65504423cb7 (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

0addrdb.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> >')
1    return addrman;
2           ^~~~~~~
3/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
4  unique_ptr(unique_ptr&& __u) _NOEXCEPT
5  ^
6./util/result.h:47:14: note: passing argument to parameter 'obj' here
7    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
8             ^
91 error generated.

DrahtBot removed the label CI failed on May 25, 2023

maflcko commented at 7:17 am on May 25, 2023: member

very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏
3FlnWlOVGKZVAPgirGB1Me+n1IHxeaKXXl2LHKPIpsnt+N2jG0ILNCy/guKB9V+K/JzCbDLy9h/UJBgDsXbspAw==

maflcko requested review from TheCharlatan on May 25, 2023

in src/txdb.h:102 in 8aa8f73adc

 98@@ -98,6 +99,6 @@ class CBlockTreeDB : public CDBWrapper
 99         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
100 };
101 
102-std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);
103+util::Result<void> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);

maflcko commented at 7:57 am on May 25, 2023:

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

TheCharlatan approved

TheCharlatan commented at 8:02 am on May 25, 2023: contributor

ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7

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

maflcko assigned ryanofsky on May 25, 2023

in src/util/result.h:46 in 8aa8f73adc

42 
43     template <typename FT>
44     friend bilingual_str ErrorString(const Result<FT>& result);
45 
46 public:
47+    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void

hebasto commented at 10:01 am on May 26, 2023:

5f49cb1bc8e6ba0671c21bf6292d2d3de43fd001

nit: Add #include <utility>?

hebasto approved

hebasto commented at 10:01 am on May 26, 2023: member

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

fanquake merged this on May 26, 2023

fanquake closed this on May 26, 2023

hebasto commented at 12:54 pm on May 26, 2023: member

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:

0$ ./configure CONFIG_SITE=$PWD/depends/arm-linux-gnueabihf/share/config.site --enable-suppress-external-warnings CXXFLAGS="-Wextra -Wno-psabi"
1$ make > /dev/null 
2addrdb.cpp: In function ‘util::Result<std::unique_ptr<AddrMan> > LoadAddrman(const NetGroupManager&, const ArgsManager&)’:
3addrdb.cpp:213:21: warning: redundant move in return statement [-Wredundant-move]
4  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)
5      |            ~~~~~~~~~^~~~~~~~~
6addrdb.cpp:213:21: note: remove ‘std::move’ call

sidhujag referenced this in commit 20272f34e0 on May 26, 2023

maflcko commented at 8:00 am on May 29, 2023: member

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

fanquake referenced this in commit 214f8f18b3 on May 30, 2023

sidhujag referenced this in commit 4b69edba9b on May 30, 2023

Fabcien referenced this in commit e0df40da76 on Dec 15, 2023

Fabcien referenced this in commit f3139ddbac on Dec 15, 2023

bitcoin locked this on May 28, 2024

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

Reviews

Conflicts

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