Skip to content

build: Require C++17 compiler #20413

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
fanquake merged 2 commits into from
Nov 19, 2020
Merged

build: Require C++17 compiler #20413

fanquake merged 2 commits into from
Nov 19, 2020

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 18, 2020

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.

@maflcko maflcko added this to the 22.0 milestone Nov 18, 2020
@jonatack
Copy link
Member

jonatack commented Nov 18, 2020

Concept ACK

1 similar comment
@practicalswift
Copy link
Contributor

practicalswift commented Nov 18, 2020

Concept ACK

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

theStack commented Nov 18, 2020

Concept ACK

@laanwj
Copy link
Member

laanwj commented Nov 18, 2020

ACK fae854da2fd8a45862e5169b698bf37a5fc57592 if it passes CI

@jnewbery
Copy link
Contributor

jnewbery commented Nov 18, 2020

Works for me.

utACK fae854da2fd8a45862e5169b698bf37a5fc57592

@practicalswift
Copy link
Contributor

practicalswift commented Nov 18, 2020

ACK fae854da2fd8a45862e5169b698bf37a5fc57592 assuming CI is happily green

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 18, 2020 •
edited
Loading

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

Conflicts

Reviewers, 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.

@DrahtBot DrahtBot mentioned this pull request Nov 18, 2020
Closed
@elichai
Copy link
Contributor

elichai commented Nov 18, 2020

utACK fac7198

hebasto
hebasto approved these changes Nov 18, 2020
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 fac7198, I've locally compiled on ARM 32bit SBC without GUI.

@maflcko
Copy link
Member Author

maflcko commented Nov 18, 2020

(ci green now)

This was referenced Nov 18, 2020
Merged
Merged
fanquake
fanquake approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fac7198

@fanquake fanquake merged commit ea79265 into bitcoin:master Nov 19, 2020
@maflcko maflcko deleted the 2011-buildCxx17 branch November 19, 2020 06:37
@maflcko maflcko mentioned this pull request Nov 19, 2020
Closed
@elichai elichai mentioned this pull request Nov 19, 2020
Closed
5 tasks
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Nov 19, 2020

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

maflcko commented Nov 19, 2020

Options:

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

@practicalswift
Copy link
Contributor

practicalswift commented Nov 19, 2020

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

@sipa
Copy link
Member

sipa commented Nov 19, 2020

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.

hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 25, 2020
@hebasto
Drop noop gcc version checks
cf580b9 
Since bitcoin#20413 the minimum required GCC version is 7.
@hebasto hebasto mentioned this pull request Nov 25, 2020
Merged
hebasto added a commit to hebasto/bitcoin that referenced this pull request Nov 25, 2020
@hebasto @practicalswift
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
@hebasto @practicalswift
Drop noop gcc version checks
a526ab4 
Since bitcoin#20413 the minimum required GCC version is 7.

Co-authored-by: practicalswift <[email protected]>
fanquake added a commit that referenced this pull request Nov 30, 2020
@fanquake
Merge #20491: refactor: Drop noop gcc version checks
817aeca 
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 30, 2020
@fanquake
Merge bitcoin#20491: refactor: Drop noop gcc version checks
888a0d5 
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
@promag promag mentioned this pull request Dec 11, 2020
Closed
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 16, 2021
@MarkLTZ
[build] Require C++17 compiler (bitcoin#20413)
26393b6
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 23, 2021
@fanquake
assumptions: assume a C++17 compiler
c7b4648 
This has already been the case since bitcoin#20413.
@fanquake fanquake mentioned this pull request Feb 23, 2021
Merged
maflcko pushed a commit that referenced this pull request Feb 23, 2021
Merge #21274: assumptions: Assume C++17
84f6c69 
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 23, 2021
Merge bitcoin#21274: assumptions: Assume C++17
6882823 
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
@hebasto hebasto mentioned this pull request Mar 24, 2021
Merged
hebasto added a commit to bitcoin-core/gui that referenced this pull request May 10, 2021
@hebasto
Merge #257: refactor: Use template function qOverload in signal-slot …
8d7125f 
…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
@hebasto hebasto mentioned this pull request Jun 23, 2021
Merged
@Bushstar Bushstar mentioned this pull request Sep 9, 2021
Closed
kwvg added a commit to kwvg/dash that referenced this pull request Sep 15, 2021
@kwvg
merge bitcoin#20413: Require C++17 compiler
140997f
christiancfifi pushed a commit to christiancfifi/dash that referenced this pull request Oct 3, 2021
@fanquake @christiancfifi
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
116dcb6 
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
christiancfifi pushed a commit to christiancfifi/dash that referenced this pull request Oct 11, 2021
@fanquake @christiancfifi
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
e447c4e 
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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@fanquake @pravblockc
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
8d0121b 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fanquake fanquake fanquake approved these changes

@hebasto hebasto hebasto approved these changes

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

22.0

Development

Successfully merging this pull request may close these issues.