build: Enable C++14 in build, require C++14 compiler.

practicalswift commented at 4:26 pm on January 25, 2019: contributor

Enable C++14 in build, require C++14 compiler.

Implements #13356.

practicalswift force-pushed on Jan 25, 2019

DrahtBot commented at 7:34 pm on January 25, 2019: 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:

#15320 ([Do Not Merge] break < Qt5.6 compatibility for addAction to test Travis 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.

in src/bench/bench_bitcoin.cpp:83 in c31324c13b outdated

79@@ -80,7 +80,7 @@ int main(int argc, char** argv)
80         return EXIT_FAILURE;
81     }
82 
83-    std::unique_ptr<benchmark::Printer> printer = MakeUnique<benchmark::ConsolePrinter>();
84+    std::unique_ptr<benchmark::Printer> printer = std::make_unique<benchmark::ConsolePrinter>();

MarcoFalke commented at 7:35 pm on January 25, 2019:

I’d prefer to defer this stylistic-only scripted diff to avoid merge conflicts

practicalswift commented at 1:23 am on January 26, 2019:

Good point. Now fixed. Leaving one use of std::make_unique to prove that we’re using C++14 :-)

practicalswift force-pushed on Jan 25, 2019

fanquake added the label Build system on Jan 25, 2019

fanquake added this to the milestone 0.19.0 on Jan 25, 2019

in src/util/memory.h:14 in 8a307fa27b outdated

 8@@ -9,11 +9,11 @@
 9 #include <memory>
10 #include <utility>
11 
12-//! Substitute for C++14 std::make_unique.
13+//! No longer needed substitute for C++14 std::make_unique. Needed when we were using C++11.
14 template <typename T, typename... Args>
15 std::unique_ptr<T> MakeUnique(Args&&... args)

ken2812221 commented at 3:37 am on January 26, 2019:

~~Could add deprecated attribute and remove them in the following few PRs.~~ Looks like MSVC would treat it as an error.

practicalswift commented at 8:29 am on January 26, 2019:

What would MSVC treat as an error? :-)

ken2812221 commented at 8:43 am on January 26, 2019:

It would throw warning C4996 and we treat it as an error in MSVC. Maybe you could add 4996 in MSVC’s warning ignore list.

practicalswift commented at 8:46 am on January 26, 2019:

@ken2812221 My suggestion is that we simply remove MakeUnique in a scripted-diff follow-up PR as soon as this PR is merged as discussed in #15262 (review). Makes sense?

laanwj commented at 10:45 am on January 28, 2019:

@ken2812221 My suggestion is that we simply remove MakeUnique in a scripted-diff follow-up PR as soon as this PR is merged as discussed in #15262 (comment). Makes sense?

To be honest, if it’s a scripted diff we could just as well do it in one go (in a sep commit) here. No need to drag this out, it’s not controversial in any way once we’ve decided to go to C++14.

practicalswift commented at 11:57 am on January 28, 2019:

@laanwj That’s how I did it initially in this PR but @MarcoFalke suggested it should be done in a follow-up PR – see comment #15262 (review) :-)

MarcoFalke commented at 3:42 pm on January 28, 2019:

The rationale behind this was that any pull requests that are expected to be open for “longer” should minimize any (silent) merge conflicts.

practicalswift commented at 7:10 pm on January 28, 2019:

I agree with that @MarcoFalke. Especially since it looks like this PR will be open for quite a while?

laanwj commented at 10:52 am on February 2, 2019:

The rationale behind this was that any pull requests that are expected to be open for “longer” should minimize any (silent) merge conflicts.

Mostly I’d agree. But OTOH if the merge conflict is in the scripted-diff part then rebasing should be trivial.

I don’t like the prospect of follow-up PR after follow-up PR. Better IMO to do it at once. But okay.

I agree with that @MarcoFalke. Especially since it looks like this PR will be open for quite a while?

Let’s merge it after the 0.18 split.

in doc/dependencies.md:10 in 8a307fa27b outdated

 6@@ -7,12 +7,12 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct
 7 | --- | --- | --- | --- | --- | --- |
 8 | Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.8.x | No |  |  |
 9 | Boost | [1.64.0](https://www.boost.org/users/download/) | [1.47.0](https://github.com/bitcoin/bitcoin/pull/8920) | No |  |  |
10-| Clang |  | [3.3+](https://llvm.org/releases/download.html) (C++11 support) |  |  |  |
11+| Clang |  | [3.5+](https://llvm.org/releases/download.html) (C++14 support) |  |  |  |

fanquake commented at 3:49 am on January 26, 2019:

Shouldn’t this be 3.4+? Looking at C++ Support in Clang:

Clang 3.4 and later implement all of the ISO C++ 2014 standard. You can use Clang in C++14 mode with the -std=c++14 option (use -std=c++1y in Clang 3.4 and earlier).

practicalswift commented at 8:23 am on January 26, 2019:

When trying out the different versions I tested by checking which versions accepted -std=c++11 and handled std::make_unique (as a sanity check). Do we want to go with the -std=c++1y route instead of -std=c++11?

Empact commented at 6:19 am on February 1, 2019:

The -std=c++1y route makes sense to me, may as well enforce that which we require.

in doc/dependencies.md:15 in 8a307fa27b outdated

12 | D-Bus | [1.10.18](https://cgit.freedesktop.org/dbus/dbus/tree/NEWS?h=dbus-1.10) |  | No | Yes |  |
13 | Expat | [2.2.6](https://libexpat.github.io/) |  | No | Yes |  |
14 | fontconfig | [2.12.1](https://www.freedesktop.org/software/fontconfig/release/) |  | No | Yes |  |
15 | FreeType | [2.7.1](https://download.savannah.gnu.org/releases/freetype) |  | No |  |  |
16-| GCC |  | [4.8+](https://gcc.gnu.org/) (C++11 support) |  |  |  |
17+| GCC |  | [4.9+](https://gcc.gnu.org/) (C++14 support) |  |  |  |

fanquake commented at 4:06 am on January 26, 2019:

I think this should be GCC 5+, see C++ Standards Support in GCC.

practicalswift commented at 8:28 am on January 26, 2019:

Ah, full C++14 support came in gcc 5 although gcc 4.9+ accepted -std=c++11. Changing. Thanks!

practicalswift force-pushed on Jan 26, 2019

MarcoFalke added the label Needs gitian build on Jan 26, 2019

DrahtBot removed the label Needs gitian build on Jan 28, 2019

practicalswift force-pushed on Feb 1, 2019

practicalswift force-pushed on Feb 2, 2019

practicalswift force-pushed on Feb 7, 2019

practicalswift force-pushed on Feb 8, 2019

ken2812221 commented at 9:50 am on February 10, 2019: contributor

You could also replace boost::shared_mutex with std::shared_timed_mutex becuase they are the same thing. https://github.com/bitcoin/bitcoin/blob/2945492424934fa360f86b116184ee8e34f19d0a/src/script/sigcache.cpp#L30

gmaxwell commented at 8:49 pm on February 11, 2019: contributor

NAK. As far as I know, C++14 is not very interesting (C++17 OTOH…) and it is still incompatible with the latest releases of widely used supported GNU/Linux distros as pointed out on #13356.

Moving the minimum required language requires balancing the negative trade-offs of incompatibility with the positive benefits of the new language features. I’m not aware of any C++14 features that would obviously improve the codebase. If there weren’t a compatibility issue then sure why not, or if there were some really important language features that would make the software safer then it might be worth the cost (as C++11 was worth some incompatibility, though it was worse than we anticipated).

MarcoFalke removed this from the milestone 0.19.0 on Feb 11, 2019

MarcoFalke added this to the milestone Future on Feb 11, 2019

MarcoFalke deleted a comment on Feb 11, 2019

practicalswift closed this on Feb 11, 2019

laanwj commented at 9:14 am on February 12, 2019: member

I agree with @gmaxwell here. Would have been nice to have this discussion (say, in a IRC meeting) before spending a lot of time polishing, but it’s true, C++14 doesn’t bring much, and will be an annoyance to users which suddenly need a newer compiler.

luke-jr commented at 2:33 pm on February 12, 2019: member

There was plenty of discussion in #13356…

MarcoFalke commented at 4:09 pm on October 1, 2019: member

Rebase, maybe?

MarcoFalke added the label Up for grabs on Oct 1, 2019

practicalswift reopened this on Oct 2, 2019

practicalswift force-pushed on Oct 2, 2019

laanwj commented at 8:04 am on October 2, 2019: member

Did anything change here? If not, I still don’t think c++14 is worth it.

practicalswift commented at 8:06 am on October 2, 2019: contributor

@laanwj This happened: #13356 (comment) + #15262 (comment)

build: Enable C++14 in build, require C++14 compiler 3e430026e2

util: Add note about obsolete MakeUnique. Use std::make_unique (C++14). 3706e8f3b3

bench: Remove workaround for gcc 4.8 type_traits issue (std::is_trivially_default_constructible) cde0fd63f1

DrahtBot added the label Needs rebase on Oct 2, 2019

doc: Bump compiler requirements for C++14 (Clang 3.4+, GCC 5+) 199cfab97a

tests: Remove Travis testing for Ubuntu 14.04 (trusty) 92e6f8219c

practicalswift force-pushed on Oct 2, 2019

DrahtBot removed the label Needs rebase on Oct 2, 2019

elichai commented at 2:36 pm on October 3, 2019: contributor

I would argue for skipping C++14 directly to 17 (assuming all stable distros ship with GCC7+ already) CC https://github.com/bitcoin/bitcoin/issues/16684

MarcoFalke commented at 2:43 pm on October 3, 2019: member

Makes sense, closing then

MarcoFalke closed this on Oct 3, 2019

MarcoFalke removed the label Up for grabs on Oct 3, 2019

MarcoFalke removed this from the milestone Future on Oct 3, 2019

laanwj commented at 10:53 am on October 8, 2019: member

Agree w.r.t. skipping directly to 17, when possible/appropriate.

On Thu, Oct 3, 2019, 16:36 Elichai Turkel notifications@github.com wrote:

I would argue for skipping C++14 directly to 17 (assuming all stable distros ship with GCC7+ already) CC #16684 https://github.com/bitcoin/bitcoin/issues/16684

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15262?email_source=notifications&email_token=AAA65NSE5HB36KSTUFLZI2LQMX7PNA5CNFSM4GSM4URKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAING3Y#issuecomment-537973615, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA65NQA7HCKM4BFCPM5CPLQMX7PNANCNFSM4GSM4URA .

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 18, 2022

build: Enable C++14 in build, require C++14 compiler. #15262

Conflicts