compile with C++11 standard and update to modern constructs

kdomanski commented at 9:23 pm on April 8, 2014: contributor

This merge will enable ISO C++ 2011 in the build process and add a BOOST define that allows proper linking of boost_filesystem, Additionally it will update the following constructs to C++11 equivalents:

replace all boost::list_of and boost::map_list_of with proper initialiser lists
replace all BOOST_FOREACH with range-based for loop
drop PAIRTYPE macro which was required by BOOST_FOREACH
spaces were added to string literals to conform with the standard
std::auto_ptr is deprecated, so it was updated to std::unique_ptr

compile with C++11 support 2e1ac16121

replace all boost::list_of with initializer lists 6f9659ddd8

fixed issue with boost scoped enums in C++11 c7aa65b7d0

changed BOOST_FOREACH etc. to C++11 range based for a223cd9749

discarded PAIRTYPE macro in favor of std::pair 2db558b279

spaces around string literals to conform with C++11 1c1c95adbc

changed deprecated std::auto_ptr to std::unique_ptr 1b28a78a99

BitcoinPullTester commented at 9:45 pm on April 8, 2014: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/1b28a78a9996795b36ebd5b1b5aff6a48f1a54d7 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 9:48 pm on April 8, 2014: member

I really wish we could pull this, but I doubt that all supported build environments are at a stage where a sufficiently compatible C++11-compiler is present (pull tester being one…).

kdomanski commented at 9:51 pm on April 8, 2014: contributor

Of course mingw is being a bitch. -.-'

sipa commented at 10:01 pm on April 8, 2014: member

For the deterministic builds of the release binaries, we’re currently using precise as build platform, which only has gcc 4.4-4.6 and clang 3.0 (and you need gcc 4.7 or 4.8, or clang 3.4 for decent C++11 support). @theuni @laanwj Can you comment on what it would take to have more modern compilers in gitian? In particular, is installing a newer version an option?

pstratem commented at 2:56 am on April 9, 2014: contributor

NACK

requires new gcc/clang not considered stable by any sane distro

maybe in a few years…

luke-jr commented at 3:01 am on April 9, 2014: member

Even if we have C++11 in gitian, we need to support compiling on stable distros too.

luke-jr commented at 3:38 am on April 9, 2014: member

Looks like GCC 4.6 (stable everywhere that matters?) actually supports everything in this PR, except std::unique_ptr, which is a library matter. Not sure how to figure out how to evaluate that.

decster commented at 4:03 am on April 9, 2014: none

I checked g++ 4.6.1(with -std=c++0x) and Apple LLVM version 5.0 (clang-500.2.79)(-std=c++11), it should be sufficient, including unique_ptr, initialiser lists, range based for

luke-jr commented at 4:10 am on April 9, 2014: member

@decster The question is the C++ stdlib version available on stable platforms.

decster commented at 5:19 am on April 9, 2014: none

I thought normally libstdc++ should be the same version with g++, right? at least in my os. std::unique_ptr is header only, maybe not depending on runtime libraries, or just use boost instead?

laanwj commented at 7:50 am on April 9, 2014: member

Looks good. If gcc4.6 can indeed support this, it’s good enough for me. Has anyone tried a gitian build with this? If not, I’ll give it a try.

We have to upgrade the pull tester environment anyway. It builds invalid Windows executables for a while already - due to hardening flags incompatible with mingw 4.4.

in src/init.cpp: in 1b28a78a99 outdated

1090     LogPrintf("nBestHeight = %d\n",                   chainActive.Height());
1091 #ifdef ENABLE_WALLET
1092-    LogPrintf("setKeyPool.size() = %"PRIszu"\n",      pwalletMain ? pwalletMain->setKeyPool.size() : 0);
1093-    LogPrintf("mapWallet.size() = %"PRIszu"\n",       pwalletMain ? pwalletMain->mapWallet.size() : 0);
1094-    LogPrintf("mapAddressBook.size() = %"PRIszu"\n",  pwalletMain ? pwalletMain->mapAddressBook.size() : 0);
1095+    LogPrintf("setKeyPool.size() = %"  PRIszu  "\n",      pwalletMain ? pwalletMain->setKeyPool.size() : 0);

laanwj commented at 8:37 am on April 9, 2014:

BTW: if you’re going to replace all lines with PRIsz* in it, you can remove those macros entirely and replace then by %*. Size specifiers are not needed with the tinyformat-based formatting that we use now (see how the macros are defined in util.h).

theuni commented at 6:19 pm on April 9, 2014: member

NACK. I was originally for this, but after a discussion on IRC last night, I’ve revisited it after learning that gcc 4.8 hasn’t landed in stable debian or gentoo yet.

C++11 is essentially a new language. Only “allowing” part of it would be a nightmare for maintenance.

C++11 cannot be used in any reasonable sense until gcc 4.8/clang 3.4. If the decision is to move towards c++11 support, this must be the floor for supported compilers. Anything less means that only a subset of the language can be used, which greatly reduces the value of the move in the first place.

If the benefits of this move are limited to the ones in this PR, or some similar subset, then the value has been missed. Arguably the biggest change to the language is that of rvalue references and move semantics, and those tend to infect code (in a good way imo) with concepts that correspond. In particular: rvalue references quickly move to perfect forwarding, which brings in variadic macros and constexpr functions as helpers. Those bring in lambdas for convenience, which pull in std::function and std::bind naturally. It keeps going from there. Same goes for type traits and SFINAE. They are hugely useful, but once they’re used in a few places, they spread out everywhere.

The above are good things imo, and very worthwhile. But many just plain don’t work until gcc4.8/clang 3.4. Imagine if (simplistic example) we had to set a policy where ‘for’ loops were allowed in code, but ‘while’ loops weren’t due to compiler support concerns. How could contributors keep up? How could reviews take place?

Tldr; This needs to be all or nothing. Either the language requirements move to c++11 or they don’t, but not some in-between concept. When the supporting compilers are ubiquitous enough for devs, that’s the time to move.

theuni commented at 6:28 pm on April 9, 2014: member

Er.. s/variadic macros/variadic templates/.

If it’s not clear from the above, I’m a huge fan of c++11 and would very much support the change when distros are ready.

laanwj commented at 6:29 pm on April 9, 2014: member

So this doesn’t work as-is on gcc 4.6?

theuni commented at 6:41 pm on April 9, 2014: member

@laanwj Whether this set of changes works or not is irrelevant. The significance here is the move to c++11.

As @luke-jr said last night, that’d be like moving from c to c++ for std::string. Everything else may continue to work, but you’ve just redefined the language that the program is written in, and must assume that people will try to use the features now at their disposal.

laanwj commented at 6:55 pm on April 9, 2014: member

That could be avoided by documenting it well, and by making the pulltester test with the correct g++ version it can be made sure that nothing is merged that wouldn’t work with the “lowest” g++ version.

Hasn’t compiler support for c++11 always been a mixed beast? Are there even now compilers that implement every single feature?

IMO even your example of switching from C to C++ for std::string would make sense if the goal is to switch to C++ fully, over time.

gavinandresen commented at 7:02 pm on April 9, 2014: contributor

I agree with @laanwj : pull-tester can quickly weed out any unsupported features. And I firmly believe in “better is better” – if we wait for perfect, complete C++11 support everywhere we will wait forever.

theuni commented at 7:02 pm on April 9, 2014: member

Tying all dev to the concept of a “correct” compiler is just not workable. I can’t review the code above without compiling it.

Yes, it has been a mixed beast. At the compiler versions I listed, that confusion practically ends.

Ok, with the std::string example, someone comes along the next day and adds a std::vector, only to find that it doesn’t work for some compilers. Switching over time just wouldn’t work.

theuni commented at 7:15 pm on April 9, 2014: member

There is no abstract ideal level. The language is a spec. Support is complete at gcc 4.8 and clang 3.4. Those are hard numbers.

There is no “waiting forever”. If you want to use those tools on your dev machine, you can install them. The only issue is whether they’re installed by default or not (a matter of convenience). If I had to pick an inconvenience for project management, I’d pick “making the devs install some 3rd-party-repo tools that conform to the language of choice” over “using a non-reviewable hybrid language” any day.

kdomanski commented at 7:17 pm on April 9, 2014: contributor

@theuni: This works perfectly with GCC-4.7 which is available on all stable distros, including the Ubuntu LTS which arrives this month. And another grand benefit here is that with a few more changes it’s possible to drop all dependency on boost except for boost::filesystem.

theuni commented at 7:40 pm on April 9, 2014: member

@kdomanski So you can replace most of boost, but it’ll require a bump of pull-tester and release-toolchain to 4.7? Aren’t you confirming my concern?

My arguments don’t come from theory. I’ve been writing c++11 exclusively for my projects. gcc 4.6/4.7 just were not practically usable. Even for supported features, they’re buggy as hell.

I’ve made my opinion clear, and repeating my points isn’t going to change any minds, so I’ll bow out of the discussion. But I don’t concede. This is a bad idea.

laanwj commented at 7:51 pm on April 9, 2014: member

As for replacing boost, wouldn’t you still need boost::spirit (for json) and boost::asio (for networking) as well?

kdomanski commented at 8:02 pm on April 9, 2014: contributor

@laanwj Correct, but still, replacing Boost is not the main objective here.

sipa commented at 8:05 pm on April 9, 2014: member

I agree with @theuni that the larger advantage to C++11 is different things than what is being used here.

I do not think that dropping Boost is viable, and if it isn’t, there is no need to attempting it. Of course, if C++11 has drop-in replacements for what Boost has, moving to those more standard solutions is nice, but just an incremental thing.

Whether it makes sense to just have the small incremental improvements, while making our build environment constraints harder… unsure.

laanwj commented at 8:13 pm on April 9, 2014: member

Seems that there is no consensus for this. Let’s try again in a few years… @kdomanski you could have saved yourself a lot of work if you discussed this with us first, for example on the mailing list :/

kdomanski commented at 8:35 pm on April 9, 2014: contributor

@sipa This PR wasn’t meant to tackle the larger advantage, just open the pathway to it.

kdomanski commented at 1:44 am on April 10, 2014: contributor

@theuni @laanwj I just tested - it works like a charm on GCC 4.6

gmaxwell commented at 2:10 am on April 10, 2014: contributor

We should also keep in mind implementation maturity. I’m not keen to be the ‘first’ to use any language feature which was just implemented in our compiler. I would like to see us advancing to a newer compiler soon for reasons unrelated to language featureset support— there have been non-trivial improvements in the reliability of the compilers in the last couple years due to some advances in testing procedures.

laanwj commented at 6:15 am on April 10, 2014: member

Upgrading to a new compiler at this point would imply changing the gitian build images to be even newer. This would bring a new glibc, libc++ and compiler. Mind that this makes the compatibility gap - which we attempt to patch over in #4042 - even larger.

kdomanski commented at 7:31 am on April 10, 2014: contributor

If your aim is compatibility with very old systems, then this pull request is misguided.

laanwj commented at 7:32 am on April 10, 2014: member

The resulting executables from gitian should run on old systems, that’s the only requirement (which is manageable if we build on 12.04 with gcc 4.6, but maybe not if we switch to 14.04 guests).

switch from boost int types to <cinttypes> 0a0a0362cf

BitcoinPullTester commented at 0:35 am on April 12, 2014: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/0a0a0362cf2ed53764f9d1a8b21a3268136099c8 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jgarzik commented at 1:21 pm on May 2, 2014: contributor

OK, closing this PR. Consensus has matched a familiar pattern which I hope serves as a template to move forward.

Bitcoin is not going to be a first mover on new language features.
boost::int64_t deserves a quick and violent death. All our platforms can handle stdint.h types such as int64_t or uint64_t. That commit in this PR useful.
Other commits, like std::pair use are good too.
In general, where older platforms permit, we should prefer the std C++ feature to a boost feature, unless there is a very clear win.
As these comments may already hint, the proper avenue for C++11 in bitcoin (or any long maintenance codebase) is to work each codebase mutation in parallel, in separate pull requests. Some changes will go right in. Other changes will not or cannot without breaking.

In general you want to form a strategy not of abrupt change, but slow change over time so that the codebase is ready by the time C++11 is widely available and tested in various Long Term Support (new fancy name for “stable”) distributions.

Most of the commits were already in that vein, but wrapping them all into a single PR was not the way to go.

jgarzik closed this on May 2, 2014

laanwj commented at 1:27 pm on May 2, 2014: member

Right, some of the commits could go in already without needing any specific C++11 support (like the std::pair one and boost::int64_t). May be worth splitting them off.

kdomanski commented at 9:46 pm on May 4, 2014: contributor

Thanks for the feedback. The whole deal with std::pair is that it needs a macro wrapper to go through BOOST_FOREACH, which only became discardable with C++11. As for the int64, I will prepare a separate merge request.

kdomanski deleted the branch on Aug 23, 2014

DrahtBot locked this on Sep 8, 2021

compile with C++11 standard and update to modern constructs #4025