Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Dec 13, 2023

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
Copy link
Contributor

DrahtBot commented Dec 13, 2023

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
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:

  • #29847 (test: add a few more base32/64 calculation corner cases by paplorinc)
  • #29432 (Stratum v2 Template Provider (take 3) by Sjors)
  • #28687 (C++20 std::views::reverse by stickies-v)
  • #26606 (wallet: Implement independent BDB parser by achow101)
  • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
  • #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
Copy link
Contributor

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
Copy link
Member Author

maflcko commented Dec 19, 2023

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
Copy link
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.

@fanquake
Copy link
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.

@vasild
Copy link
Contributor

vasild commented Feb 27, 2024

Shouldn't our Span be dropped completely, now that we have C++20 and std::span?

@maflcko
Copy link
Member Author

maflcko commented Feb 27, 2024

Yes, this change is required to move from Span toward std::span. (Clarified in the description)

@vasild
Copy link
Contributor

vasild commented Feb 28, 2024

Concept ACK

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants