build: Require C++17 compiler by maflcko · Pull Request #20413 · bitcoin/bitcoin

maflcko · 2020-11-18T09:58:44Z

Developers have been compiling with C++17 for a few months now (fuzz tests and the msvc build have it even enabled by default). According to #16684, the 22.0 release shall be compiled with C++17 enabled.

This only sets the build flag, any other changes need more discussion and can be done later.

jonatack · 2020-11-18T10:29:53Z

Concept ACK

practicalswift · 2020-11-18T10:50:00Z

Concept ACK

theStack · 2020-11-18T11:14:24Z

Concept ACK

laanwj · 2020-11-18T11:21:56Z

ACK fae854da2fd8a45862e5169b698bf37a5fc57592 if it passes CI

jnewbery · 2020-11-18T11:28:22Z

Works for me.

utACK fae854da2fd8a45862e5169b698bf37a5fc57592

practicalswift · 2020-11-18T11:43:33Z

ACK fae854da2fd8a45862e5169b698bf37a5fc57592 assuming CI is happily green

DrahtBot · 2020-11-18T11:43:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

ci: Run ci configs on cirrus #19179 (ci: Run ci configs on cirrus by MarcoFalke)
test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013 (test: add v0.20.1 to backwards compatibility test by Sjors)

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.

elichai · 2020-11-18T14:20:27Z

utACK fac7198

hebasto

ACK fac7198, I've locally compiled on ARM 32bit SBC without GUI.

maflcko · 2020-11-18T19:44:16Z

(ci green now)

fanquake

ACK fac7198

jonasschnelli · 2020-11-19T19:19:33Z

Bitcoinbuilds is failing since this (ubuntu 18.04, clang-8, native qt/libs)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/basic_string.tcc:1271:25 in
https://bitcoinbuilds.org/index.php?ansilog=cde36fc5-66ef-4f85-a6c2-c7efb509bc01.log#l1987

maflcko · 2020-11-19T19:54:17Z

Options:

Use focal (like our ci configs)
Use a newer compiler (not tested) g++-8 or clang++-10
Add a suppression
Something else?

practicalswift · 2020-11-19T20:35:19Z

I suggest adding a suppression since this isn't in our code.

sipa · 2020-11-19T22:28:08Z

unsigned integer overflow is not UB. It's just enabled because it is a red flag - but in standard libraries I think we can assume this is fine.

Since bitcoin#20413 the minimum required GCC version is 7.

Since bitcoin#20413 the minimum required GCC version is 7. Co-authored-by: practicalswift <[email protected]>

830ddf4 Drop noop gcc version checks (Hennadii Stepanov) Pull request description: Since #20413 the minimum required GCC version is 7. ACKs for top commit: fanquake: ACK 830ddf4 Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964

830ddf4 Drop noop gcc version checks (Hennadii Stepanov) Pull request description: Since bitcoin#20413 the minimum required GCC version is 7. ACKs for top commit: fanquake: ACK 830ddf4 Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964

This has already been the case since bitcoin#20413.

5e531e6 assumptions: check C++17 assumption with MSVC (fanquake) c7b4648 assumptions: assume a C++17 compiler (fanquake) Pull request description: This has been the case since #20413. This should also enable the check for MSVC. From my reading of https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-160 and https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ if we set the `/Zc:__cplusplus` switch in additional options, MSVC will report the correct value for `__cplusplus`. However I have not tested this. ACKs for top commit: laanwj: Code review ACK 5e531e6 hebasto: ACK 5e531e6, checked the MS docs, and AppVeyor build is green. practicalswift: ACK 5e531e6 Tree-SHA512: a4fb525cf5c33abc944c614edb0313a39c8a39a1637a03c09342c15ba0925f4eb037062e65e51b42ade667506b7e554c7159acf86e6b8c35d0a87dd79a6f239b

5e531e6 assumptions: check C++17 assumption with MSVC (fanquake) c7b4648 assumptions: assume a C++17 compiler (fanquake) Pull request description: This has been the case since bitcoin#20413. This should also enable the check for MSVC. From my reading of https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-160 and https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ if we set the `/Zc:__cplusplus` switch in additional options, MSVC will report the correct value for `__cplusplus`. However I have not tested this. ACKs for top commit: laanwj: Code review ACK 5e531e6 hebasto: ACK 5e531e6, checked the MS docs, and AppVeyor build is green. practicalswift: ACK 5e531e6 Tree-SHA512: a4fb525cf5c33abc944c614edb0313a39c8a39a1637a03c09342c15ba0925f4eb037062e65e51b42ade667506b7e554c7159acf86e6b8c35d0a87dd79a6f239b

…connections cdbc2bd qt: Use template function qOverload in signal-slot connections (Hennadii Stepanov) Pull request description: A nice template function [`qOverload`](https://doc.qt.io/qt-5/qtglobal.html#qOverload) is available for us now (bitcoin/bitcoin#20413, bitcoin/bitcoin#21286). Its usage makes code much more readable. This PR does not change behavior. ACKs for top commit: Talkless: utACK cdbc2bd. promag: Code review ACK cdbc2bd. Tree-SHA512: 72002aa646b1a79bab62d498825b3f245dc7ebdc189280f8bd3b4076e1bb50be8802c02bc872ff6f70c1ea81faec66d3bec36471119dd98c9e70d87b990396ae

830ddf4 Drop noop gcc version checks (Hennadii Stepanov) Pull request description: Since bitcoin#20413 the minimum required GCC version is 7. ACKs for top commit: fanquake: ACK 830ddf4 Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964

maflcko added this to the 22.0 milestone Nov 18, 2020

fanquake added the Build system label Nov 18, 2020

maflcko force-pushed the 2011-buildCxx17 branch 2 times, most recently from 7b8d2f0 to fae854d Compare November 18, 2020 10:56

DrahtBot mentioned this pull request Nov 18, 2020

Depends: Update FreeType package (CVE-2020-15999) #20320

Closed

MarcoFalke added 2 commits November 18, 2020 15:15

build: Require C++17 compiler

faaee81

Use std::make_unique

fac7198

maflcko force-pushed the 2011-buildCxx17 branch from fae854d to fac7198 Compare November 18, 2020 14:17

hebasto approved these changes Nov 18, 2020

View reviewed changes

This was referenced Nov 18, 2020

ci: Run ci configs on cirrus #19179

Merged

test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013

Merged

fanquake approved these changes Nov 19, 2020

View reviewed changes

fanquake merged commit ea79265 into bitcoin:master Nov 19, 2020

maflcko deleted the 2011-buildCxx17 branch November 19, 2020 06:37

maflcko mentioned this pull request Nov 19, 2020

build: Re-enable i686 qt build on centos 8 #20423

Closed

elichai mentioned this pull request Nov 19, 2020

Replace boost::filesystem with std::filesystem #19245

Closed

5 tasks

hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 25, 2020

Drop noop gcc version checks

cf580b9

Since bitcoin#20413 the minimum required GCC version is 7.

hebasto mentioned this pull request Nov 25, 2020

refactor: Drop noop gcc version checks #20491

Merged

hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 25, 2020

Drop noop gcc version checks

830ddf4

Since bitcoin#20413 the minimum required GCC version is 7. Co-authored-by: practicalswift <[email protected]>

hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 25, 2020

Drop noop gcc version checks

a526ab4

Since bitcoin#20413 the minimum required GCC version is 7. Co-authored-by: practicalswift <[email protected]>

promag mentioned this pull request Dec 11, 2020

RFC: Migrate from Qt5 to Qt6's Qt5 compat #20627

Closed

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 16, 2021

[build] Require C++17 compiler (bitcoin#20413)

26393b6

fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 23, 2021

assumptions: assume a C++17 compiler

c7b4648

This has already been the case since bitcoin#20413.

fanquake mentioned this pull request Feb 23, 2021

assumptions: Assume C++17 #21274

Merged

hebasto mentioned this pull request Mar 24, 2021

refactor: Use template function qOverload in signal-slot connections bitcoin-core/gui#257

Merged

hebasto mentioned this pull request Jun 23, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

Bushstar mentioned this pull request Sep 9, 2021

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Sep 15, 2021

merge bitcoin#20413: Require C++17 compiler

140997f

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

maflcko commented Nov 18, 2020

Uh oh!

jonatack commented Nov 18, 2020

Uh oh!

practicalswift commented Nov 18, 2020

Uh oh!

theStack commented Nov 18, 2020

Uh oh!

laanwj commented Nov 18, 2020

Uh oh!

jnewbery commented Nov 18, 2020

Uh oh!

practicalswift commented Nov 18, 2020

Uh oh!

DrahtBot commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

elichai commented Nov 18, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 18, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 19, 2020

Uh oh!

maflcko commented Nov 19, 2020

Uh oh!

practicalswift commented Nov 19, 2020

Uh oh!

sipa commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

DrahtBot commented Nov 18, 2020 •

edited

Loading