Skip to content
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.

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

[wip] build: Switch from C++11 to C++17 #17904

Closed
wants to merge 8 commits into from

Conversation

@practicalswift
Copy link
Member

practicalswift commented Jan 10, 2020

Switch from C++11 to C++17.

Fixes #16684 ("Discussion: upgrading to C++17").

This is work in progress and thus not ready to merge. C++17 is optimistically planned for 0.21.0.

  • build: Update ax_cxx_compile_stdcxx.m4 to latest version which supports C++17 checking
  • build: Use C++17 mode when compiling. Require a compiler with support for C++17 language features.
  • ci: Add -fsanitize=integer suppression (unsigned-integer-overflow) for libstdc++ C++17's basic_string (rfind)
  • Use std::make_unique (C++14) instead of legacy MakeUnique wrapper
  • Use std::optional (C++17) instead of boost::optional
  • Use [[nodiscard]] (C++17) instead of legacy NODISCARD macro

Left to do:

  • Make AppVeyor compile using MSVC in C++17 mode.
  • Revert temporary commit "Temporarily disable Travis build jobs without a C++17 compiler" and apply proper fix instead
  • Revert temporary commit "Temporarily disable test_spanparsing" (test_spanparsing does not not compile in C++17 mode) and apply proper fix instead
@MarcoFalke MarcoFalke added this to the 0.21.0 milestone Jan 10, 2020
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 10, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17900 (ci: Combine 32-bit build with CentOS 7 build by theStack)
  • #17894 (tests: Avoid accumulating allocated memory in a global state if LogPrintf is called when fuzzing by practicalswift)
  • #17891 (scripted-diff: Replace CCriticalSection with RecursiveMutex by MarcoFalke)
  • #17822 (refactor: Use uint16_t instead of unsigned short by ahook)
  • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
  • #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
  • #17737 (Add ChainstateManager, remove BlockManager global by jamesob)
  • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
  • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
  • #17577 (refactor: deduplicate the message sign/verify code by vasild)
  • #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
  • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
  • #17482 (util: Disallow network-qualified command line options by ryanofsky)
  • #17477 (Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals by jnewbery)
  • #17458 (Refactor OutputGroup effective value calculations and filtering to occur within the struct by achow101)
  • #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
  • #17398 (build: Update leveldb to 1.22+ by laanwj)
  • #17331 (Use effective values throughout coin selection by achow101)
  • #17268 (Epoch Mempool by JeremyRubin)
  • #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
  • #17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
  • #16698 (Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
  • #16673 (Relog configuration args on debug.log rotation by LarryRuane)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
  • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
  • #16490 (rpc: Report reason for replaceable txpool transactions by MarcoFalke)
  • #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
  • #16411 (BIP-325: Signet support by kallewoof)
  • #16377 ([rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction by Sjors)
  • #16115 (On bitcoind startup, write config args to debug.log by LarryRuane)
  • #15606 ([experimental] UTXO snapshots by jamesob)
  • #15590 (Descriptor: add GetAddressType() and IsSegWit() by Sjors)
  • #12134 (Build previous releases and run functional tests by Sjors)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)
  • #9381 (Remove CWalletTx merging logic from AddToWallet 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.

@@ -23,7 +23,8 @@ define $(package)_set_vars
$(package)_config_opts_release = -release
$(package)_config_opts_debug = -debug
$(package)_config_opts += -bindir $(build_prefix)/bin
$(package)_config_opts += -c++std c++11
# Note: C++17 not supported yet.
$(package)_config_opts += -c++std c++14

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 11, 2020

Contributor

I'm not 100% sure on this but this answer says that it should work at Qt 5.12:
https://stackoverflow.com/questions/46610996/cant-use-c17-features-using-g-7-2-in-qtcreator

Not sure if all distros support Qt 5.12

@practicalswift practicalswift force-pushed the practicalswift:c++17 branch from 5640c44 to 236eb06 Jan 12, 2020
@elichai

This comment has been minimized.

Copy link
Contributor

elichai commented Jan 13, 2020

I'd say let's stick to build changes only here. code changes can come in a follow up PR
(as there's a lot more changes that can be done with 17)

EDIT: I meant if and when it will happen. I think it's still early to even change the build system for C++17 support.

Copy link
Member

fanquake left a comment

I think it's a bit too early to open this PR. Having a branch with the changes is interesting, and would make a good discussion point for #16684.

However as a PR, it's going to be a long time until this is might be merged, and in the interim it's going to conflict with a lot of other PRs, and generate a lot of noise.

I've left a couple comments inline where you've dropped our changes to the ax_cxx_compile_stdcxx.m4 macro. Not sure if that was intentional.

AC_LANG_PUSH([C++])dnl
ac_success=no
m4_if([$4], [nodefault], [], [dnl

This comment has been minimized.

Copy link
@fanquake

fanquake Jan 13, 2020

Member

You've dropped our changes here, was that intentional? If so, can you explain why they are no-longer required as part of the commit.

dnl Require C++11 compiler (no GNU extensions)
AX_CXX_COMPILE_STDCXX([11], [noext], [mandatory], [nodefault])
dnl Require C++17 compiler (no GNU extensions)
AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory], [nodefault])

This comment has been minimized.

Copy link
@fanquake

fanquake Jan 13, 2020

Member

If dropping our .m4 changes was intentional (see above), this call needs to be updated, as it no longer takes 4 arguments.

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented Jan 13, 2020

@fanquake The ax_cxx_compile_stdcxx.m4 was taken as-is from our upstream: I didn't know we had intentional deviations from upstream, so the reversion of those local changes was unintentional :)

Closing PR for now: I see your point about DrahtBot notifications due to merge conflict. Feel free to take on this PR as 0.21.0 is getting closer to merge - it is up for grabs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.