This is for CI and devs only and doesn't change that C++17 is the standard we are currently using. The option --enable-c++20 allows CI to check that the C++17 code in the repo is also valid C++20. (There are some cases where valid C++17 doesn't compile under C++20).
Also, it allows developers to easily play with C++20 in the codebase.
You may or may not be interested in one of my PRs to dash that did this https://github.com/dashpay/dash/pull/4600
I was planning on doing the equivalent here, but didn't want to until we could merge #23340 which if I recall correctly is required to fix cxx20 builds
Concept ACK
Support for c++20 has now been merged into the ax_cxx_compile_stdcxx.m4 macro upstream: https://github.com/autoconf-archive/autoconf-archive/pull/244.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
If you want to split the macro update off, happy to merge that earlier, given it's a mechanical change.
utACK
Concept ACK. I think having the developer option, and carrying some number of code changes to support c++20 compilation is fine, but it's going to be a long time before we can make it a requirement.
Force pushed to address feedback: #24531 (review)
50 | @@ -51,12 +51,26 @@ class path : public std::filesystem::path 51 | // Disallow std::string conversion method to avoid locale-dependent encoding on windows. 52 | std::string string() const = delete; 53 | 54 | + std::string u8string() const
@ryanofsky or @hebasto you might be interested in the fs changes here?
Code review ACK fa442133f1ea37edb7c769590da9592b541fce6c. All these changes look safe to me, and it is definitely better if the code is tested with c++20 and compiles without changes.
A lot of changes are made in the PR without an explanation about the reason, though. Like the enum-enum warning change and scheduler lambda change. Ideally the changes would be broken up more, with one change per commit, and the commit messages would explain the reasons behind the changes, or at least give hints.
Thx, done. (No code changes)
Code review ACK fab63ca14298d37022e23d3e9747bc4bf94121c9. Thanks for the descriptions, I understand this much better now!
Concept and code-review ACK fab63ca14298d37022e23d3e9747bc4bf94121c9 :two: :zero:
Tested --enable-c++20 compilation with clang 11.1.0 (OpenBSD 7.0).
Concept ACK.
I'd say the two pull requests are mostly orthogonal. If one is merged before the other, the one merged second can drop the temporary -Wno commit.
ACK fab63ca14298d37022e23d3e9747bc4bf94121c9
I'd say the two pull requests are mostly orthogonal. If one is merged before the other, the one merged second can drop the temporary
-Wnocommit.
Agree.
Removed a commit. Should be trivial to re-ACK.
~re-ACK fa6bc3b2ccb7386ed496dd8be6b748e95c28b957~
CI failure?
<strike>Please review #24641 first, while this fails to compile.
Without the changes, g++ will warn to compile under C++20:
scheduler.cpp:114:21: warning: implicit capture of ‘this’ via ‘[=]’ is deprecated in C++20 [-Wdeprecated]
114 | scheduleFromNow([=] { Repeat(*this, f, delta); }, delta);
| ^
scheduler.cpp:114:21: note: add explicit ‘this’ or ‘*this’ capture
Without the changes, the file will fail to compile under C++20 because
char8_t can not be converted to char implicitly.
This makes code that uses the helper less verbose.
Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Code review ACK 999982b06ce1d1280e5ce48f9253c6c536f41a12. Since last review was rebased, and enum-conversion change was dropped, and CSerializedNetMsg copy workaround was added
utACK 999982b06ce1d1280e5ce48f9253c6c536f41a12
