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

stickies-v · 2023-10-19T12:37:13Z

C++20 introduces std::ranges::views::reverse, which allows us to drop our own reverse_iterator.h implementation and also makes it easier to chain views (even though I think we currently don't use this).

DrahtBot · 2023-10-19T12:37:17Z

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	achow101, maflcko

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:

#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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.

fanquake · 2023-10-19T12:39:29Z

cc @sipa you might have some insight here in regards to prevector.

maflcko · 2023-12-11T17:49:26Z

WIP: our prevector implementation requires modification for std::ranges::views::reverse to work. More specifically: it needs to implement the bidirectional_range concept, which currently it seems not to (see static_assert(std::ranges::bidirectional_range<prevector>) output below) as we're not satisfying range. Once that's done, we can update the last usage of reverse_iterator.h here and here and remove the header entirely.

That is also required by std::span.

stickies-v · 2023-12-11T18:51:49Z

Needs rebase

Thx, rebased to include #28349 & updated PR description

fanquake · 2023-12-12T11:21:10Z

https://github.com/bitcoin/bitcoin/pull/28687/checks?check_run_id=19528202548

/usr/bin/ccache clang++-13 -stdlib=libc++ -std=c++20 -DHAVE_CONFIG_H -I. -I../src/config  -fmacro-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /ci_container_base/depends/x86_64-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE   -isystem /ci_container_base/depends/x86_64-pc-linux-gnu/include -pthread -I/ci_container_base/depends/x86_64-pc-linux-gnu/include  -I/ci_container_base/depends/x86_64-pc-linux-gnu/include/  -fdebug-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection   -Werror    -fvisibility=hidden -fPIE -pipe -std=c++20 -O2  -c -o libbitcoin_node_a-net_processing.o `test -f 'net_processing.cpp' || echo './'`net_processing.cpp
net_processing.cpp:2074:62: error: no member named 'reverse' in namespace 'std::views'
            for (const uint256& hash : vHashes | std::views::reverse) {
                                                 ~~~~~~~~~~~~^
net_processing.cpp:2760:69: error: no member named 'reverse' in namespace 'std::views'
            for (const CBlockIndex *pindex : vToFetch | std::views::reverse) {
                                                        ~~~~~~~~~~~~^
2 errors generated.

Looks like ranges didn't become complete / non-experimental in LLVM until version 16: https://releases.llvm.org/16.0.0/projects/libcxx/docs/ReleaseNotes.html

The C++20 ranges library has been completed and is no longer experimental (with the exception of ranges::join_view which is still marked as experimental because it is about to undergo an ABI-breaking change in the Standard due to D2770). Work on C++23 ranges has started.

Note that prior versions of LLVM required distros/venders to compile with special flags, so I assume it's just not going to be generally available until 16? i.e from the LLVM 14 release notes:

More parts of ranges have been implemented. Since we still expect to make some API and ABI breaking changes, those are disabled by default. However, vendors that wish to enable in their distribution may do so by defining -DLIBCXX_ENABLE_INCOMPLETE_FEATURES=ON when configuring their build.

maflcko · 2023-12-12T12:03:00Z

Hmm, there's no clang-16 in the current LTS release of Ubuntu, so I guess this will have to wait for some time.

https://packages.ubuntu.com/jammy/clang-16

I wonder whether std::span will be usable, given that it internally assumes ranges?

DrahtBot · 2023-12-13T22:23:38Z

Guix builds (on x86_64)

File	commit `f0e8290` (master)	commit `2a92fac` (master and this pull)
SHA256SUMS.part	`7997d8e0220138cc...`	`3e66821911a14934...`
*-aarch64-linux-gnu-debug.tar.gz	`c2d1d518af6b6690...`	`71cd2844ef1594e9...`
*-aarch64-linux-gnu.tar.gz	`24b6219549eb167f...`	`62103870da8375e6...`
*-arm-linux-gnueabihf-debug.tar.gz	`43d2f9e984064b30...`	`95c903521454832f...`
*-arm-linux-gnueabihf.tar.gz	`62029d495b06ae94...`	`2bec136fd3d597e5...`
*-arm64-apple-darwin-unsigned.tar.gz	`5940ccb123abcb6f...`	`80071654801bc8bb...`
*-arm64-apple-darwin-unsigned.zip	`5f9ad735e43975c2...`	`cadd46af1fd698b9...`
*-arm64-apple-darwin.tar.gz	`4fb329d17e653ace...`	`09bc2dd4f449aaa0...`
*-powerpc64-linux-gnu-debug.tar.gz	`b89080b50e79f418...`	`1e1c09db73cbc3cf...`
*-powerpc64-linux-gnu.tar.gz	`958122ba55627d09...`	`71e4c7cea959d564...`
*-powerpc64le-linux-gnu-debug.tar.gz	`3b193a6facbfaca1...`	`55c83a7292bcdef5...`
*-powerpc64le-linux-gnu.tar.gz	`133288419542c1bc...`	`c1987c5dc27c19a0...`
*-riscv64-linux-gnu-debug.tar.gz	`497e98ac63114289...`	`6ec70782d4801883...`
*-riscv64-linux-gnu.tar.gz	`46bbcb20a8f21119...`	`7d0da4d25a6f60c5...`
*-x86_64-apple-darwin-unsigned.tar.gz	`ab343d4c4677f67c...`	`64c41cb674a66651...`
*-x86_64-apple-darwin-unsigned.zip	`ac24b8017268700b...`	`99a2559f0b3845b0...`
*-x86_64-apple-darwin.tar.gz	`eb8a6f37e0bb38de...`	`0b2972137b3a2a6b...`
*-x86_64-linux-gnu-debug.tar.gz	`6b25557dcc1c1ac2...`	`31655748d794c403...`
*-x86_64-linux-gnu.tar.gz	`7edcc22d840d9362...`	`9d6ba3a813312f33...`
*.tar.gz	`846962d7c9f05011...`	`d351b109a6580b69...`
guix_build.log	`7c8dff6c68055a73...`	`79e8210da9e68cc4...`
guix_build.log.diff		`1520c94a16f066b4...`

maflcko · 2023-12-14T08:41:18Z

Interesting that the guix macOS build passed, given that it is on clang-15, no?

fanquake · 2023-12-14T09:51:35Z

Interesting that the guix macOS build passed, given that it is on clang-15, no?

I'd guess this is because our macOS SDK (libc++) is from Xcode 15, which is based on LLVM ~16, and includes support for ranges. So as long as clang-15 can compile it, which it should given vanilla LLVM supports the use of ranges (experimentally) in 15, it probably works.

maflcko · 2023-12-14T10:19:27Z

Oh, I guess clang-15 can use libc++-16, but not libstdc++, according to https://godbolt.org/z/dbKWeE8Te and llvm/llvm-project@babdef2 ?

maflcko · 2023-12-20T10:04:39Z

Though, on GHA it says XCode 14.3, see https://github.com/bitcoin/bitcoin/actions/runs/7171999335/job/19528203033?pr=28687#step:3:14

clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: x86_64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Applications/Xcode_14.3.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

fanquake · 2023-12-20T10:09:54Z

Though, on GHA it says XCode 14.3

According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3.

maflcko · 2024-01-23T07:50:14Z

WIP: our prevector implementation requires modification for std::ranges::views::reverse to work. More specifically: it needs to implement the bidirectional_range concept, which currently it seems not to (see static_assert(std::ranges::bidirectional_range<prevector>) output below) as we're not satisfying range. Once that's done, we can update the last usage of reverse_iterator.h here and here and remove the header entirely.

That is also required by std::span.

(Possibly?) fixed in #29275

fanquake · 2024-01-24T15:05:46Z

(Possibly?) fixed in #29275

Looks like it does, this compiles for me locally atleast: https://github.com/fanquake/bitcoin/tree/28687_29275.

@stickies-v can you rebase this branch on top of #29275.

stickies-v · 2024-06-12T10:40:06Z

Force pushed to address merge conflict from #28830 and rebase on top of #30263

stickies-v · 2024-07-08T17:27:03Z

With #30263 merged, this PR is now ready for review 🥳

Use std::ranges::views::reverse instead of the implementation in reverse_iterator.h, and remove it as it is no longer used.

stickies-v · 2024-08-05T23:25:07Z

Force pushed to address merge conflict from #28052

achow101 · 2024-08-08T18:54:32Z

ACK 2925bd5

maflcko · 2024-08-09T06:52:00Z

ACK 2925bd5 🎷

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d 🎷
hdWxKaQlmycdmMRya28H4AKptHOQLgq//CDoWCwS6QAA5hcnhTRD1nkFbYpGtK6moizOMjisBVHALD8qIuYNAA==

TheCharlatan · 2024-08-09T08:56:13Z

Post-merge ACK 2925bd5

…ammy`) 1592a0f ci: update containers and CI to use Ubuntu 22.04 LTS (`jammy`) (Kittywhiskers Van Gogh) decbd05 ci: stop running `check-symbols` during builds (Kittywhiskers Van Gogh) Pull request description: ## Motivation While some aspects of C++20 are supported by GCC 9.3 (the version shipped with `focal`, [source](https://packages.ubuntu.com/focal/g++)), P0896R4 ("The One Ranges Proposal") is not ([source](https://en.cppreference.com/w/cpp/compiler_support)), required for moving away from [dash#4622](#4622) to adopt `std::ranges` and for backporting PRs like [bitcoin#28687](bitcoin#28687). They're supported from GCC 10.1 onwards ([source](https://stackoverflow.com/questions/56118941/do-we-have-c20-ranges-library-in-gcc-9/56118997#56118997)) and the next available LTS, `jammy`, currently ships with GCC 11.2 ([source](https://packages.ubuntu.com/jammy/g++)). As we're now using Guix to build our releases, there shouldn't be any adverse effects from our containers having a higher version of `glibc`. Additionally, upstream uses 24.04 (`noble`) for their build images ([source](https://github.com/bitcoin/bitcoin/blob/a2317e27b7fb86df4e32cd1674c06e09cb808248/ci/test/00_setup_env_native_tsan.sh#L10)) and `jammy` to determine the minimum version to CMake ([source](https://github.com/bitcoin/bitcoin/blob/a2317e27b7fb86df4e32cd1674c06e09cb808248/CMakeLists.txt#L5-L6)) (for reference, `focal` ships with CMake 3.16, [source](https://packages.ubuntu.com/focal/cmake)). ## Additional Information * Builds will no longer run `check-symbols` (and the option to do so, `RUN_SYMBOL_TESTS` has been removed) as portable builds (builds expected for distributions) are built with Guix, which pins its version of `glibc` ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/guix/manifest.scm#L152), currently 2.28), runs `check-symbols` ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/guix/libexec/build.sh#L304-L305)), which in turn make sure that it doesn't contain symbols expected in versions higher than `glibc` 2.31 ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/devtools/symbol-check.py#L38-L42)). Upstream does not use `check-symbols` during their builds nor do we have a reason to (as we no longer using Gitian). * CI build failures w.r.t. `pthread_yield` ([build](https://github.com/dashpay/dash/actions/runs/11645989968/job/32437272177?pr=6379#step:7:3136), [build](https://github.com/dashpay/dash/actions/runs/11645989968/job/32430165640#step:7:3157)) should be resolved by rebuilding `bdb4` (see [bitcoin#26423](bitcoin#26423 (comment))). This problem has only been observed on GitHub CI and locally (the latter resolved by rebuilding depends, I use a separate target triple for testing, `x86_64-jammy-linux-gnu`), GitLab CI has not observed such failures ([build](https://gitlab.com/dashpay/dash/-/jobs/8253943710)). ## Breaking changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1592a0f knst: utACK 1592a0f PastaPastaPasta: utACK 1592a0f Tree-SHA512: cbe6505211246c711dc0fd8645b8a4c6123b5ac163341dca4c686f8905a630c57d483a91a6e29bd9e23bac79d774e339181d2cb17bb3affeb5902e5f548ffa54

, bitcoin#29181, bitcoin#29085, bitcoin#29484, bitcoin#29577, bitcoin#29081, bitcoin#29815, bitcoin#28687, bitcoin#31502, partial bitcoin#26707, bitcoin#28894 (c++20 backports: part 2) 2867107 merge bitcoin#31502: Fix CXXFLAGS on NetBSD (Kittywhiskers Van Gogh) 83e8ff0 merge bitcoin#28687: std::views::reverse (Kittywhiskers Van Gogh) 525bf6f merge bitcoin#29815: always use our fallback timingsafe_bcmp rather than libc's (Kittywhiskers Van Gogh) 42aa57a merge bitcoin#29081: Remove gmtime* (Kittywhiskers Van Gogh) d5179ec refactor: move away from `gmtime` (Kittywhiskers Van Gogh) 3a71354 merge bitcoin#29577: ignore deprecated-declarations warnings in objc++ macOS code (Kittywhiskers Van Gogh) 74bcdf0 merge bitcoin#29484: replace char-is-int8_t autoconf detection with c++20 concept (Kittywhiskers Van Gogh) e317a5c merge bitcoin#29085: Use std::rotl (Kittywhiskers Van Gogh) 9f2c278 merge bitcoin#29181: remove systemtap variadic patch (Kittywhiskers Van Gogh) 209488d merge bitcoin#29040: Remove pre-C++20 code, fs::path cleanup (Kittywhiskers Van Gogh) e2eb625 partial bitcoin#28894: batch all individual spkms setup db writes in a single db txn (Kittywhiskers Van Gogh) f29610b partial bitcoin#26707: Fix `performance-*move*` warnings in headers (Kittywhiskers Van Gogh) ee63d4b merge bitcoin#24470: Disallow more unsafe string->path conversions allowed by path append operators (Kittywhiskers Van Gogh) abfd303 merge bitcoin#24469: Correctly decode UTF-8 literal string paths (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6378 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 2867107 UdjinM6: utACK 2867107 Tree-SHA512: 9ade297d5078063229c297036c36ba2fd82eb0ffc15124d746564844315b6ed9d86f38ddc0f4e9f00e12f8d5eeeb6248c2a86b65a7bee856a234071e1546cf40

DrahtBot added the CI failed label Oct 19, 2023

This was referenced Oct 19, 2023

Cluster mempool implementation #28676

Open

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

blockstorage: XOR blocksdir *.dat files #28052

Merged

DrahtBot mentioned this pull request Nov 17, 2023

Drop CAutoFile #28904

Merged

DrahtBot added the Needs rebase label Nov 22, 2023

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from 1cf6617 to 8ed1c24 Compare November 22, 2023 21:48

DrahtBot removed the Needs rebase label Nov 22, 2023

DrahtBot added the Needs rebase label Dec 1, 2023

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from 8ed1c24 to 4eec99d Compare December 2, 2023 12:23

DrahtBot removed the Needs rebase label Dec 2, 2023

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from 4eec99d to 95f9af8 Compare December 11, 2023 18:50

stickies-v changed the title ~~[POC][WIP] C++20 std::views::reverse~~ [WIP] C++20 std::views::reverse Dec 11, 2023

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

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

fanquake mentioned this pull request Dec 14, 2023

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

Closed

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from de52629 to f81565f Compare April 29, 2024 17:59

DrahtBot removed the Needs rebase label Apr 29, 2024

DrahtBot mentioned this pull request May 24, 2024

[refactor] Check CTxMemPool options in ctor #28830

Merged

DrahtBot mentioned this pull request Jun 10, 2024

build: Bump clang minimum supported version to 16 #30263

Merged

DrahtBot added the Needs rebase label Jun 11, 2024

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch 2 times, most recently from a7911f6 to 2d493d8 Compare June 12, 2024 10:10

DrahtBot removed the Needs rebase label Jun 12, 2024

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from 2d493d8 to 47539d3 Compare July 8, 2024 15:43

stickies-v marked this pull request as ready for review July 8, 2024 17:26

stickies-v mentioned this pull request Jul 8, 2024

fuzz: bound some miniscript operations to avoid fuzz timeouts #30197

Merged

DrahtBot added the Needs rebase label Aug 5, 2024

refactor: use c++20 std::views::reverse instead of reverse_iterator.h

2925bd5

Use std::ranges::views::reverse instead of the implementation in reverse_iterator.h, and remove it as it is no longer used.

stickies-v force-pushed the 2023-10/cpp20-use-ranges-reverseview branch from 47539d3 to 2925bd5 Compare August 5, 2024 23:23

DrahtBot removed the Needs rebase label Aug 6, 2024

fanquake merged commit 24ced52 into bitcoin:master Aug 9, 2024
16 checks passed

stickies-v deleted the 2023-10/cpp20-use-ranges-reverseview branch August 9, 2024 09:25

kwvg added a commit to kwvg/dash that referenced this pull request Nov 2, 2024

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

7ac249e

kwvg mentioned this pull request Nov 2, 2024

ci: update containers and CI to use Ubuntu 22.04 LTS (jammy) dashpay/dash#6379

Merged

5 tasks

kwvg added a commit to kwvg/dash that referenced this pull request May 5, 2025

merge bitcoin#28687: std::views::reverse

bb0d575

kwvg added a commit to kwvg/dash that referenced this pull request May 6, 2025

merge bitcoin#28687: std::views::reverse

83e8ff0

bitcoin locked and limited conversation to collaborators Aug 9, 2025

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

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

Uh oh!

Conversation

stickies-v commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

fanquake commented Oct 19, 2023

Uh oh!

maflcko commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v commented Dec 11, 2023

Uh oh!

fanquake commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 12, 2023

Uh oh!

DrahtBot commented Dec 13, 2023

Guix builds (on x86_64)

Uh oh!

maflcko commented Dec 14, 2023

Uh oh!

fanquake commented Dec 14, 2023

Uh oh!

maflcko commented Dec 14, 2023

Uh oh!

maflcko commented Dec 20, 2023

Uh oh!

fanquake commented Dec 20, 2023

Uh oh!

maflcko commented Jan 23, 2024

Uh oh!

fanquake commented Jan 24, 2024

Uh oh!

stickies-v commented Jun 12, 2024

Uh oh!

stickies-v commented Jul 8, 2024

Uh oh!

stickies-v commented Aug 5, 2024

Uh oh!

achow101 commented Aug 8, 2024

Uh oh!

maflcko commented Aug 9, 2024

Uh oh!

Uh oh!

TheCharlatan commented Aug 9, 2024

Uh oh!

Uh oh!

stickies-v commented Oct 19, 2023 •

edited

Loading

DrahtBot commented Oct 19, 2023 •

edited

Loading

maflcko commented Dec 11, 2023 •

edited

Loading

fanquake commented Dec 12, 2023 •

edited

Loading