Skip to content
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

Clang 17 compatibility #69

Merged

Conversation

vkrause
Copy link
Member

@vkrause vkrause commented Feb 20, 2024

See individual commits for details, mainly fixing new warnings that break the build due to -Werror being set here.

That however only fixes half of the overall problem, date still needs a solution. That is unlikely to get upstream fixes for C++20 compatibility it seems (as it itself has become part of C++20 in slightly modified form), but with that new API appearing in libc++ we end up with a few clashes and/or ambiguities.

Clang 17 warns about this by default and with -Werror enabled here that
breaks the build.
One of those, unknown_category, is shadowed by a local static variable
in service::display_name(), which Clang 17 warns about and with -Werror
breaks the build.
@felixguendling felixguendling merged commit adb7af6 into motis-project:master Feb 20, 2024
12 checks passed
@felixguendling
Copy link
Member

Thank you!

Regarding the date library, the only change I did was to embed the IANA timezone database into the binary using our res library. This is useful to not depend on a installed timezone database or having to link cURL and rely on a working internet connection. I didn't check how the standard implementation accesses the timezone database on different systems. In my opinion, it would make sense to continue to embed the database into the binary.

@vkrause
Copy link
Member Author

vkrause commented Feb 20, 2024

I don't think those changes are the problem here, it's mainly clashing ostream operator<< overloads it seems, upstream also has an open issues about that (HowardHinnant/date#773).

@felixguendling
Copy link
Member

I was thinking about how to upgrade to the std:: version of the date library. However, looking at the compiler support it seems that the support is still "partial" which is not something I really want to experiment with - especially not if we can just stick to the date library which is really solid. Since we have our own fork of the date library anyway, we might as well fix the problems there.

A similar problem exists with the fmt library: moving to the std:: version is not really worth it. The fmt library also gets performance improvements and new features here and there. So the std:: version doesn't seem to be superior.

@vkrause
Copy link
Member Author

vkrause commented Mar 4, 2024

motis-project/date#2 addresses the date problem.

@vkrause vkrause deleted the work/vkrause/clang-17-compat branch March 4, 2024 18:27
@oviano
Copy link

oviano commented Jun 16, 2024

It's definitely a challenge trying to use this excellent library with C++20 when targeting various platforms:

Android - doesn't have zones, formatting or writing of durations and time points to streams
iOS/macOS - no zones, but does have formatting and streaming
Linux - has streaming
Windows - has everything

Obviously the above is specific to the versions of compilers I am using, but generally the most recent official.

@felixguendling
Copy link
Member

I guess I lack some context.

Regrading targets: currently we support MacOS (AppleClang), Windows (MSVC) and Linux (Clang 17 + GCC 13 for amd64 + aarch64) which is also tested via CI both in MOTIS as well as here. More targets (such as Android / iOS) are definitely possible (probably with slight changes here and there) but currently there's no use case so nobody invested the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants