refactor: Avoid copies in FlatSigningProvider Merge #25748

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2207-merge-🚗 changing 6 files +19 −24
  1. MarcoFalke commented at 12:40 PM on July 30, 2022: member

    Merge will create several copies unconditionally:

    • To initialize the args a, and b
    • ret, which is the merge of the two args

    So change the code to let the caller decide how many copies they need/want:

    • a, and b must be explicitly moved or copied by the caller
    • ret is no longer needed, as a can be used for it in place "for free"
  2. fanquake added the label Refactoring on Jul 30, 2022
  3. DrahtBot commented at 2:01 PM on July 30, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24149 (Signing support for Miniscript Descriptors by darosior)
    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

    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.

  4. achow101 commented at 3:53 PM on August 4, 2022: member

    Related is #16116. Could you also pull in the benchmark from that?

  5. achow101 commented at 6:58 PM on August 8, 2022: member

    ACK fa00d8f84dca8ad9c5a3580e0f7a027f472cdc41

  6. achow101 commented at 6:59 PM on August 8, 2022: member

    Silent merge conflict with master

  7. DrahtBot added the label Needs rebase on Aug 11, 2022
  8. MarcoFalke referenced this in commit bf3f05f41d on Aug 12, 2022
  9. MarcoFalke force-pushed on Aug 12, 2022
  10. refactor: Avoid copies in FlatSigningProvider Merge fa3f15f2dd
  11. MarcoFalke force-pushed on Aug 12, 2022
  12. DrahtBot removed the label Needs rebase on Aug 12, 2022
  13. MarcoFalke commented at 3:54 PM on August 12, 2022: member

    Rebased and used @Empact's idea to transform this into a member function from https://github.com/bitcoin/bitcoin/pull/16116/files (thanks), which is a lot faster than an elided copy and std::move(). I've kept the argument as r-ref to allow stealing the pointers, which again should be faster than copying the entries individually.

  14. furszy approved
  15. furszy commented at 4:00 PM on August 12, 2022: member

    looks good, ACK fa3f15f2

  16. achow101 commented at 4:03 PM on August 12, 2022: member

    ACK fa3f15f2dd94ae597a66037f5928fe4e90fe099d

  17. sidhujag referenced this in commit f35b6d28c6 on Aug 12, 2022
  18. ryanofsky approved
  19. ryanofsky commented at 8:40 PM on August 17, 2022: contributor

    Code review ACK fa3f15f2dd94ae597a66037f5928fe4e90fe099d. Confirmed that all the places std::move was added the argument actually did seem safe to move from. Compiler enforces that temporary copies are explicitly created in non-move cases.

  20. achow101 merged this on Aug 17, 2022
  21. achow101 closed this on Aug 17, 2022

  22. sidhujag referenced this in commit 8e42a8dda3 on Aug 18, 2022
  23. MarcoFalke deleted the branch on Aug 19, 2022
  24. bitcoin locked this on Aug 19, 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: 2026-04-17 06:13 UTC

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