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

pull practicalswift wants to merge 5 commits into bitcoin:master from practicalswift:c++14 changing 14 files +27 −31
  1. practicalswift commented at 4:26 pm on January 25, 2019: contributor

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

    Implements #13356.

  2. practicalswift force-pushed on Jan 25, 2019
  3. 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.

  4. 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 :-)
  5. practicalswift force-pushed on Jan 25, 2019
  6. practicalswift force-pushed on Jan 25, 2019
  7. fanquake added the label Build system on Jan 25, 2019
  8. fanquake added this to the milestone 0.19.0 on Jan 25, 2019
  9. 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.

  10. 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.
  11. 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!
  12. practicalswift force-pushed on Jan 26, 2019
  13. MarcoFalke added the label Needs gitian build on Jan 26, 2019
  14. DrahtBot removed the label Needs gitian build on Jan 28, 2019
  15. practicalswift force-pushed on Feb 1, 2019
  16. practicalswift force-pushed on Feb 1, 2019
  17. practicalswift force-pushed on Feb 2, 2019
  18. practicalswift force-pushed on Feb 7, 2019
  19. practicalswift force-pushed on Feb 8, 2019
  20. 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
  21. 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).

  22. MarcoFalke removed this from the milestone 0.19.0 on Feb 11, 2019
  23. MarcoFalke added this to the milestone Future on Feb 11, 2019
  24. MarcoFalke deleted a comment on Feb 11, 2019
  25. practicalswift closed this on Feb 11, 2019

  26. 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.
  27. luke-jr commented at 2:33 pm on February 12, 2019: member
    There was plenty of discussion in #13356
  28. MarcoFalke commented at 4:09 pm on October 1, 2019: member
    Rebase, maybe?
  29. MarcoFalke added the label Up for grabs on Oct 1, 2019
  30. practicalswift reopened this on Oct 2, 2019

  31. practicalswift force-pushed on Oct 2, 2019
  32. 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.
  33. practicalswift commented at 8:06 am on October 2, 2019: contributor
  34. build: Enable C++14 in build, require C++14 compiler 3e430026e2
  35. util: Add note about obsolete MakeUnique. Use std::make_unique (C++14). 3706e8f3b3
  36. bench: Remove workaround for gcc 4.8 type_traits issue (std::is_trivially_default_constructible) cde0fd63f1
  37. DrahtBot added the label Needs rebase on Oct 2, 2019
  38. doc: Bump compiler requirements for C++14 (Clang 3.4+, GCC 5+) 199cfab97a
  39. tests: Remove Travis testing for Ubuntu 14.04 (trusty) 92e6f8219c
  40. practicalswift force-pushed on Oct 2, 2019
  41. DrahtBot removed the label Needs rebase on Oct 2, 2019
  42. 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
  43. MarcoFalke commented at 2:43 pm on October 3, 2019: member
    Makes sense, closing then
  44. MarcoFalke closed this on Oct 3, 2019

  45. MarcoFalke removed the label Up for grabs on Oct 3, 2019
  46. MarcoFalke removed this from the milestone Future on Oct 3, 2019
  47. 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 .

  48. practicalswift deleted the branch on Apr 10, 2021
  49. DrahtBot locked this on Aug 18, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-27 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me