Discussion: Upgrading to C++20 #23363

issue maflcko openend this issue on October 26, 2021
  1. maflcko commented at 3:22 pm on October 26, 2021: member

    (Previous discussion (C++17): #16684)

    With package managers shipping newer versions of compilers and older releases of operating systems going EOL, it occurs that at some point in the future it will be almost uncontroversial to switch to C++20.

    See the new features here: https://en.cppreference.com/w/cpp/compiler_support#cpp20

    I think noteworthy are:

    Overall this doesn’t look like massive improvements, so switching to C++20 is probably low(est) priority.

  2. maflcko added the label Brainstorming on Oct 26, 2021
  3. maflcko added this to the milestone Future on Oct 26, 2021
  4. sipa commented at 3:42 pm on October 26, 2021: member

    Given the new build process, it is fairly easy to use a newer compiler, while still targetting older systems, right?

    If so, I think it makes sense to fairly early on have an optional C++20 mode, and have it enabled in one of the CI targets. This makes sure the source code is C++20 compatible, and perhaps use some C++20 features where possible. It could also be enabled for releases, if there is a significant benefit.

    I don’t think we want to migrate to have the code depend on C++20 features very soon, as self-compilation on non-cutting-edge systems should remain supported.

  5. maflcko commented at 3:53 pm on October 26, 2021: member
    It looks like designated initializers were partially implemented in gcc 4.7 and clang 3.0, meaning we can enable C++20 compilation with our current dependencies of gcc-8 and clang-7, without raising them and without affecting any users?
  6. maflcko added the label Build system on Oct 26, 2021
  7. ajtowns commented at 9:51 pm on October 26, 2021: contributor
    C++20 ranges are pretty cool too. Like @sipa says, I figured we could do compatibility tests anytime, but wouldn’t be able to rely on the new features until 2023 or 2024 or so.
  8. sipa commented at 9:56 pm on October 26, 2021: member

    I’m not really a fan of very granularly whitelisting features; it sounds like it’d just be confusing. Even if we only want designated initializers, we can’t restrict gcc/clang to only permit those, so you could end up in a situation where inadvertently someone introduces a dependency on a C++20 feature that happens to be supported in all compilers used in CI, but not in all compilers we want to support.

    Regarding ranges… yes, but that kind of feature doesn’t give us much. Having conditional code that either uses ranges or a fallback, sounds like it’d generally be more complex and error-prone than just not using the ranges interface at all.

  9. maflcko commented at 8:36 am on October 27, 2021: member

    I don’t think we can avoid an “incomplete” support of the C++ language version. Currently we support C++17, but can use neither std::filesystem, nor std::set::merge. Similarly, when we switch to C++20, we won’t be able to support std::format (and other stuff that hasn’t been implemented at all in any compiler).

    Also, our minimum compiler support is now checked by CI, so it shouldn’t be possible to introduce a C++ feature that isn’t supported by the compilers we want to support.

    So I think when and if we turn on C++20, partial support of C++20 features will be expected and unavoidable.

  10. sipa commented at 6:31 pm on October 27, 2021: member
    @MarcoFalke That’s fair, but I think there is still a significant difference between “most of C++20, except these things” and “only this and this and this feature”.
  11. maflcko removed this from the milestone Future on Dec 3, 2021
  12. maflcko added this to the milestone 24.0 on Dec 3, 2021
  13. maflcko commented at 11:16 am on December 3, 2021: member
    Ok, I’ve changed the milestone to 24.0 for now, at which point we can consider adding a --enable-c++20 option for developers and CI, similar to #18591 Edit: Done in #24169
  14. maflcko removed this from the milestone 24.0 on Mar 9, 2022
  15. maflcko commented at 1:25 pm on March 30, 2022: member

    Closing for now. It doesn’t look like this gives us any new features at this point, as the wanted features are already implemented by hand by us in C++17.

    Feel free to continue discussion or ask for a reopen.

  16. maflcko closed this on Mar 30, 2022
  17. maflcko commented at 10:49 am on June 29, 2023: member
    Looks like gcc can be bumped from the now minimum required 9.1 to 10, to get roughly all of C++20. Given that all recent LTS releases of all supported operating systems ship with such a compiler, maybe this can be considered? Maybe not for 26.x, but 27.x should be fine?
  18. maflcko reopened this on Jun 29, 2023
  19. maflcko added this to the milestone 27.0 on Jun 29, 2023
  20. ajtowns commented at 1:20 am on August 19, 2023: contributor
    Having std::span<> with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like assert(key.size() == KEYLEN).
  21. theuni commented at 1:29 pm on August 22, 2023: member

    c++20 would allow us to get rid of the autoconf-tested and platform-specific conversion code used by serialize.h.

    The current complicated macro/posix-ish approach is a problem for libbitcoinkernel, specifically because it makes bitcoin-config.h a part of the api (which is definitely a bug). There are other ways to fix this, but simply making it header-only with c++20 would be the cleanest.

    See here for a POC.

  22. fanquake commented at 3:30 pm on August 30, 2023: member

    c++20 would allow us to get rid of the autoconf-tested and platform-specific conversion code used by serialize.h.

    This would be very nice to be able todo. Looks like we are also already in the clear in terms of minimum required versions of compilers and standard libraries.

  23. fanquake referenced this in commit 3e691258d8 on Dec 8, 2023
  24. fanquake commented at 11:54 am on December 12, 2023: member
    Going to close this now that #28349 has been merged, and work is already underway to use C++20 code. I think further discussion about specific C++20 features, and their usage / compiler requirements, can continue in their own issues.
  25. fanquake closed this on Dec 12, 2023
  26. maflcko commented at 8:45 am on December 15, 2023: member

    Having std::span<> with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like assert(key.size() == KEYLEN).

    I guess this requires a helper method to safely convert to a fixed-size span, internally preferring a compile-time check, or falling back to an assert, if it isn’t available?

  27. ajtowns commented at 4:18 pm on December 15, 2023: contributor

    Having std::span<> with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like assert(key.size() == KEYLEN).

    I guess this requires a helper method to safely convert to a fixed-size span, internally preferring a compile-time check, or falling back to an assert, if it isn’t available?

    It doesn’t /require/ it, but it’d probably be sensible so we always get an assertion failure rather than corrupted memory?

    I think you only need a helper for dynamic spans; for compile-time spans, just using foo.subspan<3,18>() already does compile time range checks. So maybe something like:

    0template <size_t N, typename T, size_t Extent>
1std::span<T,N> MkFixedSpan(std::span<T,Extent> src)
2{
3    static_assert(Extent == std::dynamic_extent, "use subspan<0,N>() for static spans");
4    assert(N <= src.size());
5    return std::span<T,N>(src);
6}
  28. maflcko commented at 4:40 pm on December 15, 2023: member 
    assert(N <= src.size());

    Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn’t needed. Currently that only works for std::array or [] types. However, it doesn’t seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.

    Ideally, this should work out of the box, but I guess it is too late to change the language now, so it may be better if we keep writing our own span implementation?

     0diff --git a/src/init.cpp b/src/init.cpp
 1index 481d5d398d..0625bce0c1 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1108,6 +1108,7 @@ bool AppInitInterfaces(NodeContext& node)
 5     return true;
 6 }
 7 
 8+#include <span>
 9 bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
10 {
11     const ArgsManager& args = *Assert(node.args);
12@@ -1933,6 +1934,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
13         client->start(*node.scheduler);
14     }
15 
16+    uint256 a1{};
17+    std::array a2{-1};
18+    std::span b1{a1};
19+    std::span b2{a2};
20+    static_assert(a2.size() == b2.size()); // OK
21+    static_assert(a1.size() == b1.size()); // Not OK
22+
23     BanMan* banman = node.banman.get();
24     node.scheduler->scheduleEvery([banman]{
25         banman->DumpBanlist();
  29. ajtowns commented at 9:06 am on December 16, 2023: contributor

    Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn’t needed. Currently that only works for std::array or [] types. However, it doesn’t seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.

    So, number one, I think you need an helper to implicitly convert a uint256 to a constant-extent span or you’ll have to be explicit everywhere:

    0class base_blob
1{
2...
3    constexpr operator std::span<unsigned char,WIDTH>() { return std::span<unsigned char,WIDTH>{*this}; }
4    constexpr operator std::span<const unsigned char,WIDTH>() const { return std::span<const unsigned char,WIDTH>{*this}; }
5};

    That’s enough to let you pass a uint256 to something that expects a 32 uchar span:

    0void foo(std::span<const unsigned char,32> data);
1void foo_u256(const uint256& num)
2{
3    foo(num);
4}

    (Then, to be able to pass a dynamic extent span to the function you probably want a helper to avoid having to repeat the type:

     0template<size_t N, typename T>
 1std::span<T,N> StaticSubSpan(const std::span<T,std::dynamic_extent>& dynspan)
 2{
 3    assert(N == dynspan.size());
 4    return std::span<T,N>(dynspan); // undefined behaviour if N != dynspan.size()
 5}
 6
 7void foo_var(std::span<const unsigned char> vardata)
 8{
 9    assert(vardata.size() == 32);
10    // foo(vardata); // NOT OK
11    // foo(vardata.subspan(32)); // NOT OK
12    // foo(vardata.subspan<32>()); // NOT OK
13    foo(StaticSubSpan<32>(vardata)); // OK
14    foo(std::span<const unsigned char, 32>(vardata)); // OK; risks undefined behaviour if vardata.size() != 32
15}

    )

    Getting back on track, if you want to assign a uint256 to a span, you can do that if you’re explicit about it being a fixed extent:

    0uint256 a_u256{};
1
2std::span<const unsigned char,32> b_u256_2{a_u256};
3static_assert(b_u256_2.extent == 32); // OK

    But if you want it to be implicit like it is for std::array, then I think you need a deduction guide:

    0namespace std {
1span(uint256& ) -> span<unsigned char, 32>;
2span(const uint256& ) -> span<const unsigned char, 32>;
3}
4std::span b_u256_1{a_u256};
5static_assert(b_u256_1.extent == 32);

    But I think adding a deduction guide for std::span on a custom class is undefined behaviour? https://stackoverflow.com/a/63424897 Maybe there’s some way to do that properly? Otherwise, being explicit doesn’t seem so bad, especially if we swapped unsigned char for uint8_t…

  30. maflcko commented at 11:11 am on December 18, 2023: member

    I think you need an helper

    Looks like you are right. I thought that it was possible to write a concept like

    0template <class T>
1concept ArrayLike = requires(T a) {
2    std::array<decltype(*a.begin()), a.size()>{};
3};

    But that doesn’t work, because a.size() is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?

  31. ajtowns commented at 11:27 am on December 18, 2023: contributor

    I think you need an helper

    Looks like you are right. I thought that it was possible to write a concept like

    0template <class T>
1concept ArrayLike = requires(T a) {
2    std::array<decltype(*a.begin()), a.size()>{};
3};

    But that doesn’t work, because a.size() is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?

    ArrayLike sounds like ranges::contiguous_range ? https://en.cppreference.com/w/cpp/ranges/contiguous_range

    The constexpr span( R&& range ); constructor is already conditional on that (ie, it’s undefined behaviour if R isn’t a contiguous_range) but that doesn’t help that much (I think) because that constructor is marked explicit for non-dynamic extents…

  32. maflcko commented at 10:41 am on December 19, 2023: member

    ArrayLike sounds like ranges::contiguous_range ? https://en.cppreference.com/w/cpp/ranges/contiguous_range

    Not exactly. contiguous_range is satisfied for std::vector<int>, whose size is runtime variable. My concept ArrayLike should be contiguous_range and checking that the size is known at compile time.

  33. ajtowns commented at 11:23 am on December 19, 2023: contributor
    https://stackoverflow.com/questions/70482497/detecting-compile-time-constantness-of-range-size ? std::span<X, N> seems like it’s already pretty much the thing that tells you you’ve got a contiguous range of exactly N X’s to me though…
  34. fanquake commented at 11:54 am on January 29, 2024: member

    On the std::format front, looks like as of the next Xcode release (15.3), Apple is adding support for std::format:

    The following new features have been implemented: P0645 - Text formatting (std::format)

  35. PastaPastaPasta referenced this in commit b2aff8d4c9 on Apr 4, 2024
  36. PastaPastaPasta referenced this in commit 27a386e77c on Apr 4, 2024
  37. PastaPastaPasta referenced this in commit 9d053303e2 on Apr 12, 2024
  38. PastaPastaPasta referenced this in commit f35a69073a on Apr 23, 2024
  39. PastaPastaPasta referenced this in commit 13ea5c4f45 on Apr 24, 2024


maflcko sipa ajtowns theuni fanquake

Labels
Brainstorming Build system

Milestone
27.0

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-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me