Skip to content

Avoid the use of P0083R3 std::set::merge #22342

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2021
Merged

Avoid the use of P0083R3 std::set::merge #22342

merged 1 commit into from
Jun 26, 2021

Conversation

sipa
Copy link
Member

@sipa sipa commented Jun 25, 2021

This use was introduced in #21365, but as pointed out in #22339, this causes compatibility problems.

Just avoid its use for now.

@sipa sipa mentioned this pull request Jun 25, 2021
Merged
jonatack
jonatack reviewed Jun 25, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3052341

@sipa sipa force-pushed the 202106_no_cxx17_merge branch from 3052341 to 6cf4ea7 Compare June 25, 2021 17:42
@jonatack
Copy link
Member

re-ACK 6cf4ea7

benthecarman
benthecarman reviewed Jun 25, 2021
View reviewed changes
Copy link
Contributor

@benthecarman benthecarman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6cf4ea7

hebasto
hebasto approved these changes Jun 26, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6cf4ea7, successfully compiled on the following systems:

  • macOS Mojave 10.14.6 (18G9216)
$ clang --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
  • Debian 9.13 (stretch)
$ make -C depends NO_QT=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'
$ ./autogen.sh && CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure -q --disable-tests --disable-bench CC=clang-7 CXX='clang++-7 -stdlib=libc++' && make clean > /dev/null
$ make

@hebasto
Copy link
Member

hebasto commented Jun 26, 2021

Should this be tagged for 22.0?

@sipa sipa added this to the 22.0 milestone Jun 26, 2021
@maflcko
Copy link
Member

maflcko commented Jun 26, 2021

Before:

$ make V=1
Making all in src
make[1]: Entering directory '/bitcoin-core/src'
make[2]: Entering directory '/bitcoin-core/src'
make[3]: Entering directory '/bitcoin-core'
make[3]: Leaving directory '/bitcoin-core'
/usr/bin/ccache clang++-7 -stdlib=libc++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./secp256k1/include  -I/bitcoin-core/depends/x86_64-pc-linux-gnu/include -I./leveldb/include -I./leveldb/helpers/memenv -I./univalue/include -I/bitcoin-core/depends/x86_64-pc-linux-gnu/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DPROVIDE_FUZZ_MAIN_FUNCTION -fdebug-prefix-map=/bitcoin-core/src=. -Wstack-protector -fstack-protector-all      -fPIE -pipe -O2  -MT script/libbitcoin_common_a-standard.o -MD -MP -MF script/.deps/libbitcoin_common_a-standard.Tpo -c -o script/libbitcoin_common_a-standard.o `test -f 'script/standard.cpp' || echo './'`script/standard.cpp
script/standard.cpp:410:22: error: no member named 'merge' in 'std::__1::set<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, ShortestVectorFirstComparator, std::__1::allocator<std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >'
        scripts[key].merge(std::move(control_blocks));
        ~~~~~~~~~~~~ ^
1 error generated.
make[2]: *** [Makefile:8408: script/libbitcoin_common_a-standard.o] Error 1
make[2]: Leaving directory '/bitcoin-core/src'
make[1]: *** [Makefile:16141: all-recursive] Error 1
make[1]: Leaving directory '/bitcoin-core/src'
make: *** [Makefile:820: all-recursive] Error 1

After: (passes)

@maflcko maflcko merged commit 9c3751a into bitcoin:master Jun 26, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 27, 2021
Merge bitcoin#22342: Avoid the use of P0083R3 std::set::merge
654f896 
6cf4ea7 Avoid the use of P0083R3 std::set::merge (Pieter Wuille)

Pull request description:

  This use was introduced in bitcoin#21365, but as pointed out in bitcoin#22339, this causes compatibility problems.

  Just avoid its use for now.

ACKs for top commit:
  jonatack:
    re-ACK 6cf4ea7
  benthecarman:
    ACK 6cf4ea7
  hebasto:
    ACK 6cf4ea7, successfully compiled on the following systems:

Tree-SHA512: 2b3fdcadb7de98963ebb0b192bd956aa68526457fe5b374c74a69ea10d5b68902763148f11abbcc471010bcdc799e0804faef5f8e8ff8a509b3a053c0cb0ba39
@fanquake
Copy link
Member

We probably could have almost left this as is for 22.0, #22339 also didn't cover the actual hard compatibility issue, which would have been needing to bump the minimum required macOS to 10.15. In any case, I think we can revert this for 0.23, as we are going to bump our macOS and lib(std)c++ requirements to use std::filesystem.

@maflcko maflcko mentioned this pull request Sep 22, 2021
Merged
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 9c3751a into merged_master (Bitcoin PR bitcoin/bitcoin#22342)
a523f13
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@jonatack jonatack jonatack left review comments

@benthecarman benthecarman benthecarman left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sipa @jonatack @hebasto @maflcko @fanquake @benthecarman