refactor: Return BResult from restoreWallet#25594
Hidden character warning
Conversation
maflcko
force-pushed
the
2207-BResult-restoreWallet-
branch
from
29f67ee to
fa869c6
Compare
maflcko
force-pushed
the
2207-BResult-restoreWallet-
branch
from
fa869c6 to
f38b370
Compare
maflcko
force-pushed
the
2207-BResult-restoreWallet-
branch
from
f38b370 to
fa475e9
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK fa869c6df15a54dae0e8d6ba385514801ddd345f
|
|
||
| public: | ||
| BResult() : m_variant(Untranslated("")) {} | ||
| BResult(const T& _obj) : m_variant(_obj) {} |
There was a problem hiding this comment.
In commit "refactor: Return BResult from restoreWallet" (fa869c6df15a54dae0e8d6ba385514801ddd345f)
I think dropping const T& constructor drops support for noncopyable and nonmovable objects.
Maybe want:
BResult(const T& obj) : m_variant{obj} {}
BResult(T&& obj) : m_variant{std::move(obj)} {}There was a problem hiding this comment.
drops support
This was never supported, unless I am missing something?
Regardless, it seems wrong to return memory that is not owned. Though, if you really want to, it might be better to make the type T explicit. That is, make it a a raw pointer/std::ref/span/string_view/whatever.
There was a problem hiding this comment.
drops support
This was never supported, unless I am missing something?
You're right. It didn't work previously or in the version I suggested either. It would be possible, but would have to call one of the std::in_place_* variant constructors here.
Regardless, it seems wrong to return memory that is not owned.
The idea would not to have BResult point to an outside value but for BResult to contain a value that is constructed in-place. But regardless, this is an edge case not worth considering here. I might support with it in my followup for #25308 and add a test if I do
There was a problem hiding this comment.
reACK fa475e9
The previous commit, although it failed some CI checks, had worked on Ubuntu 21.10 clang version 13.0.0-2.
Maybe shared_ptr<CWallet> src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,...) might also be changed to BResult<CWallet> src/wallet/wallet.h:std::RestoreWallet(..)
|
I proposed some changes to |
Good idea. Feel free to ping me for review on a follow-up. For now I am trying to keep the changes here to a minimum. |
|
|
||
| public: | ||
| BResult() : m_variant(Untranslated("")) {} | ||
| BResult(const T& _obj) : m_variant(_obj) {} |
There was a problem hiding this comment.
drops support
This was never supported, unless I am missing something?
You're right. It didn't work previously or in the version I suggested either. It would be possible, but would have to call one of the std::in_place_* variant constructors here.
Regardless, it seems wrong to return memory that is not owned.
The idea would not to have BResult point to an outside value but for BResult to contain a value that is constructed in-place. But regardless, this is an edge case not worth considering here. I might support with it in my followup for #25308 and add a test if I do
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
fa475e9 refactor: Return BResult from restoreWallet (MacroFake) fa8de09 Prepare BResult for non-copyable types (MacroFake) Pull request description: This avoids the `error` in-out param (and if `warnings` is added to `BResult`, it will avoid passing that in-out param as well). Also, as it is needed for this change, prepare `BResult` for non-copyable types. ACKs for top commit: w0xlt: reACK bitcoin@fa475e9 ryanofsky: Code review ACK fa475e9. Changes since last review were replacing auto with explicit type and splitting commits Tree-SHA512: 46350883572f13721ddd198f5dfb88d2fa58ebcbda416f74da3563ea15c920fb1e6ff30558526a4ac91c36c21e6afe27751a4e51b7b8bcbcbe805209f4e9014b
07df6cd wallet: Return `util::Result` from WalletLoader methods (w0xlt) Pull request description: This PR adds a method that implement common logic to WalletLoader methods and change them to return `BResult<std::unique_ptr<Wallet>>`. Motivation: #25594 changed `restoreWallet` to return `BResult` but this method shares a common pattern with `createWallet` and `loadWallet`. This PR keeps the same pattern to all WalletLoader methods. ACKs for top commit: jonatack: Review ACK 07df6cd theStack: Code-review ACK 07df6cd Tree-SHA512: 2fe321134883f7cce60206888113800afef0fa168dab184e1a8692cd21a231970eb9c25c7220ea39e5d457134002d47f0974463925db76abbf8dfcd421629c63
…r methods 07df6cd wallet: Return `util::Result` from WalletLoader methods (w0xlt) Pull request description: This PR adds a method that implement common logic to WalletLoader methods and change them to return `BResult<std::unique_ptr<Wallet>>`. Motivation: bitcoin#25594 changed `restoreWallet` to return `BResult` but this method shares a common pattern with `createWallet` and `loadWallet`. This PR keeps the same pattern to all WalletLoader methods. ACKs for top commit: jonatack: Review ACK 07df6cd theStack: Code-review ACK 07df6cd Tree-SHA512: 2fe321134883f7cce60206888113800afef0fa168dab184e1a8692cd21a231970eb9c25c7220ea39e5d457134002d47f0974463925db76abbf8dfcd421629c63
5 participants
This avoids the
errorin-out param (and ifwarningsis added toBResult, it will avoid passing that in-out param as well).Also, as it is needed for this change, prepare
BResultfor non-copyable types.