Conversation
|
Looking at it now, I see we had some custom overrides in |
8b54014 to
65b6f30
Compare
c06de7c to
02ccf78
Compare
|
Looks like this failed last time because of |
|
The test case "LedgerTxnRoot prefetch classic entries" fails because vectors of Edit: |
173bce7 to
a7b50fd
Compare
|
I think these failures are related to the
|
bboston7
left a comment
There was a problem hiding this comment.
Nice work! This seems like the upgrade requires only minimal changes to the C++ code, and I agree with the modifications you made. It looks like some docs still need updating. INSTALL.md and README.md both still reference C++17.
70ac2be to
6e98897
Compare
There was a problem hiding this comment.
Pull request overview
This PR upgrades the codebase from C++17 to C++20. The changes include updating compiler configuration, resolving deprecated API usage, and adapting to C++20's stricter requirements for format strings and template syntax.
Changes:
- Updated build configuration and documentation to require C++20 instead of C++17
- Replaced deprecated
std::result_ofwithstd::invoke_result_tin autocheck library - Wrapped non-constexpr format strings with
fmt::runtime()to satisfy C++20 requirements - Updated submodule versions for
fmt,spdlog, andxdrppto versions compatible with C++20
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| configure.ac | Updated compiler standard version requirement from 17 to 20 |
| src/util/XDROperators.h | Replaced operator< with spaceship operator operator<=> |
| src/util/Logging.h | Removed FMT_STRING wrapper for format strings (C++20 handles compile-time strings automatically) |
| src/util/Logging.cpp | Wrapped runtime format pattern with fmt::runtime() and replaced fmt::localtime() with standard library |
| src/util/DebugMetaUtils.h | Changed format string to constexpr char array for compile-time string literal |
| src/main/test/ConfigTests.cpp | Converted string format patterns to constexpr and wrapped runtime patterns with fmt::runtime() |
| src/ledger/LedgerEntryScope.h | Removed template argument from deleted constructor (C++20 DR 2237 compliance) |
| src/history/HistoryArchive.cpp | Wrapped runtime format template with fmt::runtime() |
| src/util/xdrquery/XDRQueryEval.cpp | Added fmt/ranges.h include for range formatting support |
| src/util/xdrquery/XDRFieldResolver.h | Added fmt/ranges.h include for range formatting support |
| src/test/test.cpp | Added fmt/ranges.h include for range formatting support |
| lib/autocheck/include/autocheck/classifier.hpp | Replaced deprecated std::result_of with std::invoke_result_t |
| m4/ax_cxx_compile_stdcxx.m4 | Updated autoconf macro to support C++20 and C++23 detection |
| m4/ax_cxx_compile_stdcxx_14.m4 | Removed unused C++14-specific macro file |
| lib/xdrpp, lib/spdlog, lib/fmt | Updated submodule commits to C++20-compatible versions |
| README.md, INSTALL.md | Updated documentation to reflect C++20 requirement |
Upgrade to C++20. Notes:
autocheckusesstd::result_of(deprecated in 17, removed in 20)—I replaced just the use of that since we seem to have a de-facto in-tree fork ofautocheck(e.g., we referencestellar::).fmt::formatrequires a either a compile-time constant first parameter in 20 or afmt::runtimecall.for
fmtto work on clang, it seems that we need to have a version starting from this commit. Otherwise, code like the following doesn't workSo, I bumped the versions of
fmtandspdlogFMT_STRINGwithout any template arguments no longer picks up the right overload, but the C++20 version seems to correctly do compile-time strings with out it, so I updatedLogging.hto reflect this. One alternative is using the following macro, but I think it's probably cleaner to just remove theFMT_STRINGusage inLogging.hxdrppneeds to get updated for two reasons.On its current master, we end up trying to build using C++14, but it should be at least 17. Bumping to 20 (or 17) we get some extra flags1
Because of the spaceship operator, at the very least, we need
operator<=>defined forxdr::pointersince it inherits fromstd::unique_ptrwhich defines<=>, meaning that without the overload, e.g.,std::vector<xdr::pointer<int>>comparisons no longer work properly (they go back to comparing by address instead of value).There is the
cxx20branch, but it doesn't have the most recent 5 commits from the master branch (which we rely on in core). Additionally, it seems to be somewhat buggy (e.g.,tests/marshal.ccdoesn't pass). So, for the purposes of this PR, I think it's probably easiest to just bump to 20 and addoperator<=>for xdr data types.Small change, but renamed the constructor in the header file for
ScopedLedgerEntrysinceScopedLedgerEntry<T>isn't technically valid since DR 2237 was adopted in C++20 (link).Other notes on C++20 compatibility. I compiled with
-Wc++20-compatlocally, and I didn't see any warnings. (Maybe we should do this in CI too?) Looking at C.1 diff.cpp17, on a version of this PR based on 414a5e5,Checked
C.1.2.3, C.1.2.5, C.1.5.4, C.1.6.1, C.1.6.2, C.1.14.1, C.1.14.4, C.1.15: The "effect on original feature" for all of these say something to the effect of "Valid C++ 2017 code... is not valid in this revision of C++" (or "may fail to compile"). Since we compile on 20, these are all okay.
C.1.5.1, C.1.8.1, C.1.9.1, C.1.14.2, C.1.14.3: The "effect on original feature" for all of these say something to the effect of "Valid C++ 2017 code may be ill-formed in this revision of C++" (n.b., not "Valid C++ 2017 code may be ill-formed in this revision of C++, with no diagnostic required."). Since we compile on 20, these are all okay.
C.1.2.1: "Effect on original feature: Logical lines beginning with
moduleorimportmay be interpreted differently in this revision of C++." The only lines we have that begin with "import" or "module" are in the updatedfmtlibraryClick to expand
(note that afaict, our codebase only has the following file endings, the others are just for posterity:
.cc,.cpp,.h,.hh,.hpp,.ipp)C.1.2.2: "Effect on original feature: When the identifier
importis followed by a<character, aheader-nametoken may be formed." These only appear in Objective-C code that we don't actually compile (incatch.hpp, they're in#ifdef __OBJC__blocks and thegtestis anxcodesubdirectoryClick to expand
C.1.2.4: "Effect on original feature: Valid C++ 2017 code that contains a
<=token immediately followed by a>token may be ill-formed or have different semantics in this revision of C++." All instances are either in our new 20 code, a comment, or a string literalClick to expand
Not (fully) checked
std::{list,forward_list}::{remove,remove_if,unique}returningvoid(instead ofcontainer::size_t).Footnotes
Due to a known issue: autoconf adds a flag to enable C++11 features since C++20 isn't fully backwards compatible. Unfortunately, the patch to fix this behavior is not in a released version of autoconf ↩