-
-
Notifications
You must be signed in to change notification settings - Fork 733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GCC 13 and C++20 support #1959
GCC 13 and C++20 support #1959
Conversation
.github/workflows/linux.yml
Outdated
- distro: fedora | ||
cpp_std: c++20 | ||
- distro: alpine | ||
ignore-test-errors: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because of this https://github.com/Alexays/Waybar/actions/runs/3943499703/jobs/6748390098#step:6:122
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. LC_TIME support on Alpine is broken, but it used to pass because std::locale
outright reported that en_US
/en_GB
were unavailable and the tests were skipped.
Something has changed in Alpine 3.17 (GCC 12?) and now it's possible to get locale facets, except that the localized date formatting is still unsupported. Thus, we have to ignore or skip the tests on Alpine.
Use smaller includes for Catch2 v3.
The structure was used to pass the locale instance to the date formatter. All the supported versions of `fmt` are passing the locale parameter via `FormatContext.locale()` so we can remove the struct and simplify the code. While we at it, drop `date::make_zoned` in favor of CTAD on a `date::zoned_time` constructor.
There were two main issues with fmtlib and C++20 mode: - `fmt::format` defaults to compile-time argument checking and requires using `fmt::runtime(format_string)` to bypass that. - `std::format` implementation introduces conflicting declarations and we have to specify the namespace for all `format`/`format_to` calls.
ed5d862
to
48b3a56
Compare
- Add tests for global locale. - Warn about missing locales. - Downgrade REQUIRE to CHECK. - Skip tests if localized formatting does not work as expected.
Use chrono Calendars and Time Zones (P0355R7, P1466R3) when available instead of the `date` library. Verified with a patched build of a recent GCC 13 snapshot.
Add an extra job to build with `-std=c++20` on Fedora. Update actions/checkout to v3.
Add some missing dependencies for Fedora.
Thanks a lot! |
I'll update dockerfiles |
GCC 13 is coming to Fedora Rawhide and it brings incomplete implementation of C++20 P0355R7 (
<chrono>
Calendars and Time Zones). It's not possible to use that with an unmodified GCC snapshot build right now, but all the code we need is already there.To compile waybar with C++20, I had to deal with compile-time format string validation in fmtlib, either by wrapping the strings with
fmt::runtime
or by making the parameterconstexpr
.We could also use
fmt::vformat
in some places and reusefmt::format_args
, but that would be prone to all kinds of lifetime bugs -fmt::arg
does not extend the lifetime of a temporary object passed as an argument.Draft because
fmt::runtime
conversion was pretty mechanical and I want to take another look at the changes. Otherwise it's ready for review.