refactor: Return BResult from restoreWallet #25594

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2207-BResult-restoreWallet-🍌 changing 4 files +18 −9
  1. MarcoFalke commented at 1:37 pm on July 12, 2022: member

    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.

  2. MarcoFalke force-pushed on Jul 12, 2022
  3. fanquake added the label Wallet on Jul 12, 2022
  4. w0xlt approved
  5. w0xlt commented at 1:58 pm on July 12, 2022: contributor

    Tested ACK https://github.com/bitcoin/bitcoin/pull/25594/commits/fa869c6df15a54dae0e8d6ba385514801ddd345f

    Perhaps this change can be split into two commits: one for BResult and one for restoreWallet.

  6. MarcoFalke force-pushed on Jul 12, 2022
  7. Prepare BResult for non-copyable types fa8de09edc
  8. refactor: Return BResult from restoreWallet fa475e9c79
  9. MarcoFalke force-pushed on Jul 12, 2022
  10. in src/util/result.h:22 in fa8de09edc outdated
    18@@ -18,9 +19,9 @@ class BResult {
    19     std::variant<bilingual_str, T> m_variant;
    20 
    21 public:
    22-    BResult() : m_variant(Untranslated("")) {}
    23-    BResult(const T& _obj) : m_variant(_obj) {}
    


    ryanofsky commented at 6:52 pm on July 12, 2022:

    In commit “refactor: Return BResult from restoreWallet” (fa869c6df15a54dae0e8d6ba385514801ddd345f)

    I think dropping const T& constructor drops support for noncopyable and nonmovable objects.

    Maybe want:

    0BResult(const T& obj) : m_variant{obj} {}
    1BResult(T&& obj) : m_variant{std::move(obj)} {}
    

    MarcoFalke commented at 7:22 am on July 13, 2022:

    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.


    ryanofsky commented at 12:46 pm on July 13, 2022:

    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

  11. ryanofsky approved
  12. ryanofsky commented at 6:56 pm on July 12, 2022: contributor
    Code review ACK fa869c6df15a54dae0e8d6ba385514801ddd345f
  13. w0xlt approved
  14. w0xlt commented at 7:19 pm on July 12, 2022: contributor

    reACK https://github.com/bitcoin/bitcoin/pull/25594/commits/fa475e9c7977a952617738f2ee8cf600c07d4df8

    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(..)

  15. w0xlt commented at 2:49 am on July 13, 2022: contributor
    I proposed some changes to BResult in #25601 that might make it easier to use in src/wallet/wallet.h:{RestoreWallet(...),CreateWallet(...),LoadWallet(...)}.
  16. MarcoFalke commented at 7:24 am on July 13, 2022: member

    Maybe shared_ptr src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,…) might also be changed to BResult src/wallet/wallet.h:std::RestoreWallet(..)

    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.

  17. ryanofsky approved
  18. ryanofsky commented at 12:50 pm on July 13, 2022: contributor
    Code review ACK fa475e9c7977a952617738f2ee8cf600c07d4df8. Changes since last review were replacing auto with explicit type and splitting commits
  19. DrahtBot commented at 4:07 am on July 14, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25608 (BResult improvements 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.

  20. MarcoFalke merged this on Jul 14, 2022
  21. MarcoFalke closed this on Jul 14, 2022

  22. bitcoin deleted a comment on Jul 14, 2022
  23. MarcoFalke deleted the branch on Jul 14, 2022
  24. sidhujag referenced this in commit b6ff16e53b on Jul 14, 2022
  25. MarcoFalke referenced this in commit f5e96ecef5 on Aug 10, 2022
  26. sidhujag referenced this in commit 8ba56bdfd0 on Aug 11, 2022
  27. bitcoin locked this on Jul 16, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me