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

maflcko · 2023-12-13T10:58:46Z

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.

DrahtBot · 2023-12-13T10:58:49Z

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.

DrahtBot · 2023-12-13T19:27:14Z

Guix builds (on x86_64)

File	commit `f0e8290` (master)	commit `7a61387` (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...`

maflcko · 2023-12-19T23:48:54Z

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?

fanquake · 2023-12-20T09:41:17Z

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.

fanquake · 2023-12-20T09:50:44Z

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.

DrahtBot · 2024-07-08T16:03:37Z

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

hodlinator

ACK fabcd0c

src/test/miniscript_tests.cpp

src/wallet/db.cpp

hodlinator

Untested re-ACK fa69fba

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

stickies-v

Approach ACK

src/span.h

Replace std::equal with std::ranges::equal, because it allows for shorter code, because no pointers or iterators have to be passed explicitly.

maflcko · 2024-08-13T08:24:34Z

Force pushed for iwyu, and to add a new (only half-related) commit.

hodlinator

Untested Code Review re-ACK fad0cf6

git range-diff master fa69fba fad0cf6 on shows #include changes and new fad0cf6 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>).

stickies-v

ACK fad0cf6

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.

stickies-v · 2024-08-13T16:04:55Z

src/test/fuzz/poolresource.cpp

@@ -78,7 +78,7 @@ class PoolResourceFuzzer
    {
        std::vector<std::byte> expect(entry.span.size());
        InsecureRandomContext(entry.seed).fillrand(expect);
-        assert(entry.span == expect);
+        assert(std::ranges::equal(entry.span, expect));


nit: missing #include <algorithm>

Leaving as-is for now to not invalidate review. I'll follow-up with iwyu on all files, I guess.

src/kernel/chainparams.cpp

TheCharlatan

ACK fad0cf6

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

maflcko · 2024-08-15T12:36:06Z

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.

maflcko · 2024-08-28T09:20:15Z

Is this rfm with three acks, or does it need more?

DrahtBot added the Refactoring label Dec 13, 2023

maflcko force-pushed the 2312-less-span- branch from fabd742 to 1ff0c38 Compare December 13, 2023 12:37

DrahtBot added the CI failed label Dec 13, 2023

maflcko marked this pull request as draft December 13, 2023 12:37

maflcko added the DrahtBot Guix build requested label Dec 13, 2023

DrahtBot removed the CI failed label Dec 13, 2023

This was referenced Dec 13, 2023

wallet: Allow users to create a wallet that encrypts all database records #28142

Closed

wallet: Implement independent BDB parser #26606

Merged

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

DrahtBot removed the DrahtBot Guix build requested label Dec 13, 2023

DrahtBot mentioned this pull request Dec 14, 2023

tracing: Only prepare tracepoint arguments when actually tracing #26593

Merged

maflcko mentioned this pull request Dec 14, 2023

build: Require libc++-16 or later #29077

Closed

maflcko force-pushed the 2312-less-span- branch from 1ff0c38 to 56c9e51 Compare December 14, 2023 10:38

maflcko added this to the 28.0 milestone Dec 15, 2023

DrahtBot mentioned this pull request Dec 15, 2023

build: Bump g++ minimum supported version to 11 #29091

Merged

maflcko mentioned this pull request Dec 19, 2023

refactor: Use std::span over Span #29119

Closed

DrahtBot mentioned this pull request Dec 20, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

This was referenced Jan 2, 2024

build: Bump clang minimum supported version to 15 #29165

Merged

Stratum v2 Template Provider (take 2) #28983

Closed

build: Bump clang minimum supported version to 14 #29208

Merged

maflcko force-pushed the 2312-less-span- branch from 56c9e51 to adaeeb2 Compare January 12, 2024 10:21

maflcko force-pushed the 2312-less-span- branch from adaeeb2 to c45d3b7 Compare February 1, 2024 17:36

DrahtBot mentioned this pull request Feb 2, 2024

C++20 std::views::reverse #28687

Merged

maflcko force-pushed the 2312-less-span- branch from c45d3b7 to 05804fb Compare February 6, 2024 11:10

DrahtBot mentioned this pull request Feb 14, 2024

Stratum v2 Template Provider (take 3) #29432

Closed

24 tasks

DrahtBot removed the Needs rebase label Jun 5, 2024

maflcko force-pushed the 2312-less-span- branch from 3a1a9f0 to 7156ebc Compare June 10, 2024 08:37

maflcko mentioned this pull request Jun 10, 2024

build: Bump clang minimum supported version to 16 #30263

Merged

maflcko marked this pull request as ready for review July 8, 2024 15:28

maflcko force-pushed the 2312-less-span- branch 2 times, most recently from fafc3a6 to fabcd0c Compare July 8, 2024 16:03

DrahtBot added the CI failed label Jul 8, 2024

DrahtBot removed the CI failed label Jul 8, 2024

hodlinator approved these changes Jul 13, 2024

View reviewed changes

src/test/miniscript_tests.cpp Outdated Show resolved Hide resolved

src/wallet/db.cpp Show resolved Hide resolved

DrahtBot requested a review from vasild July 13, 2024 13:31

maflcko force-pushed the 2312-less-span- branch from fabcd0c to fa69fba Compare August 9, 2024 10:18

fanquake requested a review from stickies-v August 9, 2024 10:54

hodlinator approved these changes Aug 9, 2024

View reviewed changes

stickies-v reviewed Aug 12, 2024

View reviewed changes

src/span.h Show resolved Hide resolved

MarcoFalke added 2 commits August 13, 2024 07:44

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

fadf0a7

refactor: Use std::ranges::equal in GetNetworkForMagic

fad0cf6

Replace std::equal with std::ranges::equal, because it allows for shorter code, because no pointers or iterators have to be passed explicitly.

maflcko force-pushed the 2312-less-span- branch from fa69fba to fad0cf6 Compare August 13, 2024 06:04

hodlinator approved these changes Aug 13, 2024

View reviewed changes

DrahtBot requested a review from stickies-v August 13, 2024 08:36

stickies-v approved these changes Aug 13, 2024

View reviewed changes

TheCharlatan approved these changes Aug 15, 2024

View reviewed changes

DrahtBot mentioned this pull request Aug 16, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

fanquake merged commit 80f00ca into bitcoin:master Aug 28, 2024
16 checks passed

maflcko deleted the 2312-less-span- branch August 28, 2024 09:37

TheCharlatan mentioned this pull request Aug 28, 2024

kernel: Use spans instead of vectors for passing block headers to validation functions #30742

Merged

maflcko mentioned this pull request Oct 15, 2024

Safegcd-based modular inverses in MuHash3072 #21590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Dec 13, 2023 •

edited

Loading

DrahtBot commented Dec 13, 2023 •

edited

Loading

DrahtBot commented Dec 13, 2023

maflcko commented Dec 19, 2023

fanquake commented Dec 20, 2023

fanquake commented Dec 20, 2023

DrahtBot commented Jul 8, 2024

hodlinator left a comment

hodlinator left a comment

stickies-v left a comment

maflcko commented Aug 13, 2024

hodlinator left a comment

stickies-v left a comment

stickies-v Aug 13, 2024

maflcko Aug 14, 2024

TheCharlatan left a comment

maflcko commented Aug 15, 2024

maflcko commented Aug 28, 2024

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

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

Conversation

maflcko commented Dec 13, 2023 • edited Loading

DrahtBot commented Dec 13, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

DrahtBot commented Dec 13, 2023

Guix builds (on x86_64)

maflcko commented Dec 19, 2023

fanquake commented Dec 20, 2023

fanquake commented Dec 20, 2023

DrahtBot commented Jul 8, 2024

hodlinator left a comment

Choose a reason for hiding this comment

hodlinator left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

maflcko commented Aug 13, 2024

hodlinator left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Aug 13, 2024

Choose a reason for hiding this comment

maflcko Aug 14, 2024

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

maflcko commented Aug 15, 2024

maflcko commented Aug 28, 2024

maflcko commented Dec 13, 2023 •

edited

Loading

DrahtBot commented Dec 13, 2023 •

edited

Loading