refactor: Remove Span operator==, Use std::ranges::equal #29071

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2312-less-span- changing 21 files +76 −74
  1. maflcko commented at 10:58 am on December 13, 2023: member

    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.

  2. DrahtBot commented at 10:58 am on December 13, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    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.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30377 (refactor: Replace ParseHex with consteval “"_hex literals by hodlinator)
    • #29847 (test: add a few more base32/64 calculation corner cases by l0rinc)
    • #29119 (refactor: Use std::span over Span by maflcko)
    • #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

    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.

  3. DrahtBot added the label Refactoring on Dec 13, 2023
  4. maflcko force-pushed on Dec 13, 2023
  5. DrahtBot added the label CI failed on Dec 13, 2023
  6. maflcko marked this as a draft on Dec 13, 2023
  7. maflcko added the label DrahtBot Guix build requested on Dec 13, 2023
  8. DrahtBot removed the label CI failed on Dec 13, 2023
  9. DrahtBot commented at 7:27 pm on December 13, 2023: contributor

    Guix builds (on x86_64)

    File commit f0e829022a415c7c9513e715c532079ec7756306(master) commit 7a6138732177be861883454ee8fd388168fc1169(master and this pull)
    SHA256SUMS.part 7997d8e0220138cc... 429492106694250c...
    *-aarch64-linux-gnu-debug.tar.gz c2d1d518af6b6690... 9c6a4ab3cfdceccd...
    *-aarch64-linux-gnu.tar.gz 24b6219549eb167f... d0fd2b8597250f41...
    *-arm-linux-gnueabihf-debug.tar.gz 43d2f9e984064b30... c1f3b0c1a6412e4f...
    *-arm-linux-gnueabihf.tar.gz 62029d495b06ae94... 623ecb84b6005704...
    *-arm64-apple-darwin-unsigned.tar.gz 5940ccb123abcb6f... e5ff73ea5d19d20a...
    *-arm64-apple-darwin-unsigned.zip 5f9ad735e43975c2... 54c4da35bae13672...
    *-arm64-apple-darwin.tar.gz 4fb329d17e653ace... 975f14ef63c8b5f7...
    *-powerpc64-linux-gnu-debug.tar.gz b89080b50e79f418... 5b5dbfe5731ebe2f...
    *-powerpc64-linux-gnu.tar.gz 958122ba55627d09... 9addf7d52452a1d9...
    *-powerpc64le-linux-gnu-debug.tar.gz 3b193a6facbfaca1... 8c22ca0b038788bb...
    *-powerpc64le-linux-gnu.tar.gz 133288419542c1bc... 350741d364942107...
    *-riscv64-linux-gnu-debug.tar.gz 497e98ac63114289... d8b667f3872eb871...
    *-riscv64-linux-gnu.tar.gz 46bbcb20a8f21119... decf0d413b786dda...
    *-x86_64-apple-darwin-unsigned.tar.gz ab343d4c4677f67c... b32f362e78af622e...
    *-x86_64-apple-darwin-unsigned.zip ac24b8017268700b... 5107e53696204924...
    *-x86_64-apple-darwin.tar.gz eb8a6f37e0bb38de... 3dde990352bc8d45...
    *-x86_64-linux-gnu-debug.tar.gz 6b25557dcc1c1ac2... 352c4f15b0bdc42c...
    *-x86_64-linux-gnu.tar.gz 7edcc22d840d9362... 1ec6bb742434455f...
    *.tar.gz 846962d7c9f05011... 6c553dc37928f9b1...
    guix_build.log 7c8dff6c68055a73... 73c7da5fea099311...
    guix_build.log.diff d11cc011e8783b31...
  10. DrahtBot removed the label DrahtBot Guix build requested on Dec 13, 2023
  11. maflcko force-pushed on Dec 14, 2023
  12. maflcko added this to the milestone 28.0 on Dec 15, 2023
  13. maflcko commented at 11:48 pm on December 19, 2023: member
    Looks like this also fails on macOS on GHA. Does anyone know what version(s) of clang can be expected to be available on macOS normally?
  14. fanquake commented at 9:41 am on December 20, 2023: member

    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.

  15. fanquake commented at 9:50 am on December 20, 2023: member

    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.

  16. maflcko force-pushed on Jan 12, 2024
  17. maflcko force-pushed on Feb 1, 2024
  18. maflcko force-pushed on Feb 6, 2024
  19. vasild commented at 10:54 am on February 27, 2024: contributor
    Shouldn’t our Span be dropped completely, now that we have C++20 and std::span?
  20. maflcko commented at 10:59 am on February 27, 2024: member
    Yes, this change is required to move from Span toward std::span. (Clarified in the description)
  21. vasild commented at 5:15 pm on February 28, 2024: contributor
    Concept ACK
  22. maflcko removed this from the milestone 28.0 on Apr 10, 2024
  23. DrahtBot added the label CI failed on Apr 20, 2024
  24. DrahtBot removed the label CI failed on Apr 26, 2024
  25. DrahtBot added the label Needs rebase on Apr 29, 2024
  26. maflcko force-pushed on Apr 29, 2024
  27. DrahtBot removed the label Needs rebase on Apr 29, 2024
  28. DrahtBot added the label Needs rebase on May 21, 2024
  29. maflcko force-pushed on Jun 5, 2024
  30. maflcko force-pushed on Jun 5, 2024
  31. DrahtBot removed the label Needs rebase on Jun 5, 2024
  32. maflcko force-pushed on Jun 10, 2024
  33. maflcko marked this as ready for review on Jul 8, 2024
  34. maflcko force-pushed on Jul 8, 2024
  35. maflcko force-pushed on Jul 8, 2024
  36. DrahtBot added the label CI failed on Jul 8, 2024
  37. DrahtBot commented at 4:03 pm on July 8, 2024: contributor

    🚧 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.

    Debug: https://github.com/bitcoin/bitcoin/runs/27171556600

  38. DrahtBot removed the label CI failed on Jul 8, 2024
  39. in src/test/miniscript_tests.cpp:21 in fabcd0c4fe outdated
    17@@ -22,6 +18,12 @@
    18 #include <script/script_error.h>
    19 #include <script/signingprovider.h>
    20 
    21+
    


    hodlinator commented at 1:08 pm on July 13, 2024:
    nit: Double newline

    maflcko commented at 10:48 am on August 9, 2024:
    Fixed. Should be trivial to re-ack
  40. in src/wallet/db.cpp:21 in fabcd0c4fe outdated
    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); }
    


    hodlinator commented at 1:26 pm on July 13, 2024:
    (Attempted switching around the parameters to lexicographical_compare() and confirmed that it broke test_bitcoin -t db_tests).
  41. hodlinator approved
  42. hodlinator commented at 1:31 pm on July 13, 2024: contributor
    ACK fabcd0c4fe44e3bc2d6a59f2839f459fd5c81171
  43. DrahtBot requested review from vasild on Jul 13, 2024
  44. maflcko force-pushed on Aug 9, 2024
  45. fanquake requested review from stickies-v on Aug 9, 2024
  46. hodlinator approved
  47. hodlinator commented at 8:00 pm on August 9, 2024: contributor

    Untested re-ACK fa69fba751079ee6042daceb88bc8f8ad3f3de96

    git range-diff master fabcd0c fa69fba reveals pure whitespace change.

  48. in src/span.h:218 in fa69fba751 outdated
    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()); }
    


    stickies-v commented at 12:10 pm on August 12, 2024:

    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
    

    maflcko commented at 6:04 am on August 13, 2024:
    • #include // lines 9-9

    This is wrong. It is used.

    Removed the other.

  49. stickies-v commented at 9:24 pm on August 12, 2024: contributor
    Approach ACK
  50. refactor: Remove Span operator==, Use std::ranges::equal fadf0a7e15
  51. refactor: Use std::ranges::equal in GetNetworkForMagic
    Replace std::equal with std::ranges::equal, because it allows for
    shorter code, because no pointers or iterators have to be passed
    explicitly.
    fad0cf6f26
  52. maflcko force-pushed on Aug 13, 2024
  53. maflcko commented at 8:24 am on August 13, 2024: member
    Force pushed for iwyu, and to add a new (only half-related) commit.
  54. hodlinator approved
  55. hodlinator commented at 8:36 am on August 13, 2024: contributor

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

  56. DrahtBot requested review from stickies-v on Aug 13, 2024
  57. in src/test/fuzz/poolresource.cpp:81 in fad0cf6f26
    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));
    


    stickies-v commented at 4:04 pm on August 13, 2024:
    nit: missing #include <algorithm>

    maflcko commented at 7:37 am on August 14, 2024:
    Leaving as-is for now to not invalidate review. I’ll follow-up with iwyu on all files, I guess.
  58. in src/kernel/chainparams.cpp:692 in fad0cf6f26
    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)) {
    


    stickies-v commented at 4:41 pm on August 13, 2024:

    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;
    

    maflcko commented at 7:36 am on August 14, 2024:
    I don’t like the suggestion, because it makes it harder to change 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.

    stickies-v commented at 10:49 am on August 14, 2024:

    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}
    

    maflcko commented at 11:10 am on August 14, 2024:

    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.

    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.

  59. stickies-v approved
  60. stickies-v commented at 4:46 pm on August 13, 2024: contributor

    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.

  61. TheCharlatan approved
  62. TheCharlatan commented at 11:50 am on August 15, 2024: contributor

    ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f

    The last commit seems a bit random, why change that function and not any of the other occurrences of std::equal?

  63. maflcko commented at 12:36 pm on August 15, 2024: member

    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.

  64. maflcko commented at 9:20 am on August 28, 2024: member
    Is this rfm with three acks, or does it need more?
  65. fanquake merged this on Aug 28, 2024
  66. fanquake closed this on Aug 28, 2024

  67. maflcko deleted the branch on Aug 28, 2024

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-22 00:12 UTC

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