std::span removed the comparison operators, so it makes sense to remove them for the Span “backport” as well. Using std::ranges::equal also has the benefit that some Span temporary constructions can now be dropped.
This is required to move from Span toward std::span.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, stickies-v, TheCharlatan |
| Concept ACK | vasild |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Looks like this also fails on macOS on GHA.
Pretty sure that job should be rerun, because it’s failing in the brew install stage.
Does anyone know what version(s) of clang can be expected to be available on macOS normally?
GIven our minimum supported macOS (11.x), you could run anything from Xcode 12.5+ onwards, which is roughly LLVM/Clang 12+. Although I’d expect most users compiling from source on macOS, would be using a much more recent Xcode, so likely Clang ~14+ at least.
Also, anyone should be able to install and run the latest LLVM/Clang via brew.
Span be dropped completely, now that we have C++20 and std::span?
Span toward std::span. (Clarified in the description)
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
17@@ -22,6 +18,12 @@
18 #include <script/script_error.h>
19 #include <script/signingprovider.h>
20
21+
18
19 namespace wallet {
20-bool operator<(BytePrefix a, Span<const std::byte> b) { return a.prefix < b.subspan(0, std::min(a.prefix.size(), b.size())); }
21-bool operator<(Span<const std::byte> a, BytePrefix b) { return a.subspan(0, std::min(a.size(), b.prefix.size())) < b.prefix; }
22+bool operator<(BytePrefix a, Span<const std::byte> b) { return std::ranges::lexicographical_compare(a.prefix, b.subspan(0, std::min(a.prefix.size(), b.size()))); }
23+bool operator<(Span<const std::byte> a, BytePrefix b) { return std::ranges::lexicographical_compare(a.subspan(0, std::min(a.size(), b.prefix.size())), b.prefix); }
lexicographical_compare() and confirmed that it broke test_bitcoin -t db_tests).
Untested re-ACK fa69fba751079ee6042daceb88bc8f8ad3f3de96
git range-diff master fabcd0c fa69fba reveals pure whitespace change.
212@@ -213,13 +213,6 @@ class Span
213 return Span<C>(m_data + m_size - count, count);
214 }
215
216- friend constexpr bool operator==(const Span& a, const Span& b) noexcept { return a.size() == b.size() && std::equal(a.begin(), a.end(), b.begin()); }
217- friend constexpr bool operator!=(const Span& a, const Span& b) noexcept { return !(a == b); }
218- friend constexpr bool operator<(const Span& a, const Span& b) noexcept { return std::lexicographical_compare(a.begin(), a.end(), b.begin(), b.end()); }
nit: no more need for #include <algorithm>
0span.h should add these lines:
1#include <utility> // for declval, forward
2
3span.h should remove these lines:
4- #include <algorithm> // lines 8-8
5- #include <cassert> // lines 9-9
6
7The full include-list for span.h:
8#include <cstddef> // for size_t, byte, nullptr_t
9#include <span> // for span
10#include <type_traits> // for remove_pointer, is_convertible, enable_if, enable_if_t, remove_cv, remove_pointer_t, false_type, true_type
11#include <utility> // for declval, forward
- #include // lines 9-9
This is wrong. It is used.
Removed the other.
Replace std::equal with std::ranges::equal, because it allows for
shorter code, because no pointers or iterators have to be passed
explicitly.
Untested Code Review re-ACK fad0cf6
git range-diff master fa69fba fad0cf6 on shows #include changes and new fad0cf6f26 commit.
git show fad0cf6 shows use of std::equal overload with 3 iterators being replaced with the generally safer std::ranges::equal (not really an issue in this specific case as we are comparing pairs of the same type MessageStartChars = std::array<uint8_t, 4>).
77@@ -78,7 +78,7 @@ class PoolResourceFuzzer
78 {
79 std::vector<std::byte> expect(entry.span.size());
80 InsecureRandomContext(entry.seed).fillrand(expect);
81- assert(entry.span == expect);
82+ assert(std::ranges::equal(entry.span, expect));
#include <algorithm>
688@@ -689,15 +689,15 @@ std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
689 const auto regtest_msg = CChainParams::RegTest({})->MessageStart();
690 const auto signet_msg = CChainParams::SigNet({})->MessageStart();
691
692- if (std::equal(message.begin(), message.end(), mainnet_msg.data())) {
693+ if (std::ranges::equal(message, mainnet_msg)) {
nit: I think we can use std::array::operator== here? (edit: or a switch statement would probably be even better)
0 if (message == mainnet_msg) {
0diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
1index 7d06d24be5..a760926ec8 100644
2--- a/src/kernel/chainparams.cpp
3+++ b/src/kernel/chainparams.cpp
4@@ -689,15 +689,15 @@ std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
5 const auto regtest_msg = CChainParams::RegTest({})->MessageStart();
6 const auto signet_msg = CChainParams::SigNet({})->MessageStart();
7
8- if (std::ranges::equal(message, mainnet_msg)) {
9+ if (message == mainnet_msg) {
10 return ChainType::MAIN;
11- } else if (std::ranges::equal(message, testnet_msg)) {
12+ } else if (message == testnet_msg) {
13 return ChainType::TESTNET;
14- } else if (std::ranges::equal(message, testnet4_msg)) {
15+ } else if (message == testnet4_msg) {
16 return ChainType::TESTNET4;
17- } else if (std::ranges::equal(message, regtest_msg)) {
18+ } else if (message == regtest_msg) {
19 return ChainType::REGTEST;
20- } else if (std::ranges::equal(message, signet_msg)) {
21+ } else if (message == signet_msg) {
22 return ChainType::SIGNET;
23 }
24 return std::nullopt;
GetNetworkForMagic to accept a span in the future. Generally, when a function accepts a read-only non-lifetimebound view of a byte blob, a span can be used. std::span supports fixed size and dynamic size, so the prior workarounds to force a fixed-size array no longer apply and it seems odd to rely on them in new code.
That makes sense, but I don’t quite see the case for changing GetNetworkForMagic to accept a span in the future, so it feels a bit like premature optimization to me, but I may be missing context.
It’s a nit, and the commit is orthogonal to the PR anyway, so this is the last I’ll comment on it - up to you what you prefer, but I feel like this would probably be the most elegant implementation:
0std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
1{
2 static const std::array<std::pair<MessageStartChars, ChainType>, 5> network_map = {{
3 {CChainParams::Main()->MessageStart(), ChainType::MAIN},
4 {CChainParams::TestNet()->MessageStart(), ChainType::TESTNET},
5 {CChainParams::TestNet4()->MessageStart(), ChainType::TESTNET4},
6 {CChainParams::RegTest({})->MessageStart(), ChainType::REGTEST},
7 {CChainParams::SigNet({})->MessageStart(), ChainType::SIGNET}
8 }};
9
10 auto it = std::ranges::find_if(network_map, [&message](const auto& pair) { return pair.first == message; });
11 return (it != network_map.end()) ? std::make_optional(it->second) : std::nullopt;
12}
That makes sense, but I don’t quite see the case for changing
GetNetworkForMagicto accept a span in the future, so it feels a bit like premature optimization to me, but I may be missing context.
Yeah, it is hard to make predictions about the future. I was mostly thinking about a DataStream or std::vector (or similar) object, and someone wanting to just call GetNetworkForMagic(std::span{obj}.first(4)), without having to write code to create a copy of the first bytes.
It is mostly about the code style and not an optimization. Happy to drop the commit, though.
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
Even though not having span comparison operators can make things a bit more verbose, these changes are needed to start using std::span over our own implementation. This approach seems like the least invasive one (cleaning up wherever possible), and I can’t see any behaviour change.
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
The last commit seems a bit random, why change that function and not any of the other occurrences of std::equal?
The last commit seems a bit random, why change that function and not any of the other occurrences of
std::equal?
Good point! I’ll address this, if I have to re-touch.