C++20 std::views::reverse #28687
C++20 std::views::reverse #28687
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
|
cc @sipa you might have some insight here in regards to prevector. |
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
1cf6617
to
8ed1c24
Compare
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
8ed1c24
to
4eec99d
Compare
That is also required by |
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
4eec99d
to
95f9af8
Compare
stickies-v
changed the title
Thx, rebased to include #28349 & updated PR description |
|
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
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:
|
|
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? |
|
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. |
|
Oh, I guess clang-15 can use libc++-16, but not libstdc++, according to https://godbolt.org/z/dbKWeE8Te and llvm/llvm-project@babdef2 ? |
|
Though, on GHA it says XCode 14.3, see https://github.com/bitcoin/bitcoin/actions/runs/7171999335/job/19528203033?pr=28687#step:3:14 |
According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3. |
(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
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
aabe87f
to
a198735
Compare
|
Force pushed to address merge conflict from #29483 |
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
a198735
to
2ea4406
Compare
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
2ea4406
to
791815d
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
Force pushed to:
|
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
791815d
to
0cef229
Compare
|
Rebased on top of #29765. One more task to go 🤞 |
stickies-v
force-pushed
the
2023-10/cpp20-use-ranges-reverseview
branch
from
0cef229
to
de52629
Compare
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
de52629
to
f81565f
Compare
C++20 introduces
std::ranges::views::reverse, which allows us to drop our ownreverse_iterator.himplementation and also makes it easier to chain views (even though I think we currently don't use this).In draft until our minimum dependencies are bumped to pass CI:
no wallet, libbitcoinkernel: requires clang-16