refactor: make EncodeBase58{Check} consume Spans #19706
Conversation
| while (pbegin != pend && *pbegin == 0) { | ||
| pbegin++; | ||
| while (input.size() > 0 && input[0] == 0) { | ||
| input = input.subspan(1); |
There was a problem hiding this comment.
Taking a subspan is a cheap operation (akin to pointer arithmetic), right?
There was a problem hiding this comment.
Yes, taking a subspan only needs to update pointer and size, which I would still consider cheap. I guess often the size update can be optimized away so that in the end only pointer arithmetic is involved, but I wouldn't feel qualified enough to give any guarantees here. Maybe @sipa can shed a bit more light on this topic.
However, I felt keen to experiment... Here is a godbolt snippet where you can see the difference of "count leading zero-bytes" implementations, on one hand implemented with pointers and on the other hand with spans (using the Bitcoin Core substitute implementation, as I couldn't find a compiler that supported C++20 std::spans yet): https://godbolt.org/z/1oGGPP The generated codes for the loop bodies are basically identical.
There was a problem hiding this comment.
Clear, I was just wondering, thank you.
|
Code review ACK 356988e |
|
Concept ACK |
356988e util: make EncodeBase58Check consume Spans (Sebastian Falbesoner) f0fce06 util: make EncodeBase58 consume Spans (Sebastian Falbesoner) Pull request description: This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs bitcoin#19660, bitcoin#19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks). ACKs for top commit: laanwj: Code review ACK 356988e Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
Summary: > util: make EncodeBase58 consume Spans > util: make EncodeBase58Check consume Spans This is a backport of [[bitcoin/bitcoin#19706 | core#19706]] No change was necessary in `bench/base58.cpp`, because we already use vectors (D1857). In `guiutil.cpp`, we use `EncodeCashAddr`. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10114
This PR improves the interfaces for the functions
EncodeBase58{Check}by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions ofEncodeBase58: one that takes two pointers (marking begin and end) and another one that takes astd::vector<unsigned char>const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done forEncodeBase58Check, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).