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

practicalswift commented at 4:07 PM on January 10, 2020: contributor

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 added this to the milestone 0.21.0 on Jan 10, 2020

MarcoFalke added the label Build system on Jan 10, 2020

DrahtBot commented at 6:15 PM on January 10, 2020: member

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.

in depends/packages/qt.mk:27 in 5640c446b6 outdated

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

emilengler commented at 8:43 AM on January 11, 2020:

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

[wip] Temporarily disable Travis build jobs without a C++17 compiler c80338a620

[wip] Temporarily disable test_spanparsing 06497a85b2

build: Update ax_cxx_compile_stdcxx.m4 to latest version which supports C++17 checking e2072147e2

ci: Add -fsanitize=integer suppression (unsigned-integer-overflow) for libstdc++ C++17's basic_string (rfind) 0b2fd443a8

build: Use C++17 mode when compiling. Require a compiler with support for C++17 language features. 064bc8bb72

Use std::optional (C++17) instead of boost::optional ebcc5fbee7

Use [[nodiscard]] (C++17) instead of legacy NODISCARD macro 4cc5db40ee

Use std::make_unique (C++14) instead of legacy MakeUnique wrapper 236eb0686d

practicalswift force-pushed on Jan 12, 2020

in build-aux/m4/ax_cxx_compile_stdcxx.m4:67 in 236eb0686d

  64 | -        [$4], [nodefault], [ax_cxx_compile_cxx$1_try_default=false],
  65 | -        [m4_fatal([invalid fourth argument `$4' to AX_CXX_COMPILE_STDCXX])])
  66 |    AC_LANG_PUSH([C++])dnl
  67 |    ac_success=no
  68 |  
  69 | -  m4_if([$4], [nodefault], [], [dnl

fanquake commented at 10:27 AM on January 13, 2020:

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.

in configure.ac:65 in 236eb0686d

  60 | @@ -61,8 +61,8 @@ case $host in
  61 |       lt_cv_deplibs_check_method="pass_all"
  62 |    ;;
  63 |  esac
  64 | -dnl Require C++11 compiler (no GNU extensions)
  65 | -AX_CXX_COMPILE_STDCXX([11], [noext], [mandatory], [nodefault])
  66 | +dnl Require C++17 compiler (no GNU extensions)
  67 | +AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory], [nodefault])

fanquake commented at 10:28 AM on January 13, 2020:

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

elichai commented at 10:30 AM on January 13, 2020: contributor

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.

fanquake commented at 10:34 AM on January 13, 2020: member

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.

practicalswift closed this on Jan 13, 2020

practicalswift commented at 12:42 PM on January 13, 2020: contributor

@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 :)

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022