-
-
Notifications
You must be signed in to change notification settings - Fork 475
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
fmtlib and C++20 #1704
Comments
Note that although we have vendored in this library, the goal is still that osm2pgsql must compile with all versions of this library that people are likely to have on their machines. Requiring specific versions of libraries isn't nice to users. |
You can try out lots different combinations of {C++ compiler, C++ standard, fmt library} versions on godbolt, and also reproduce the github actions issue there: https://cpp.godbolt.org/z/fofvroPKr or https://cpp.godbolt.org/z/Kja6o7Eqa Any particular reason you want to stick with UDL, rather than switching to fmt::format? |
The way I understood it in older versions of fmtlib you got compile time checking of format strings only when using the UDL syntax (and then only with newer C++ versions). I am not sure about that though, fmt lib docs for the older version are not available online any more. It took me a while back then to get all of it working properly and I don't want to spend the time again trying to figure out what the right way of doing things is that works across all versions. It really is a big guessing game unless the fmtlib people document what the right strategy is for backwards and forwards compatibility. If you can research this and find a proper solution that would be great! Otherwise I think we'll just muddle through for another two or three years until we can use C++20 which comes with std::format and ditch fmtlib. |
I am working on fixing this now, this will need a few PRs. In most places where performance is not an issue, we'll simply switch to |
This replaces most uses of the ""_format literal with function calls to fmt::format(). The latter form is available in all versions of the fmt library, while the ""_format literal is deprecated in version 8 and removed in version 9. This might cost us some performance and early error detection, because in at least some versions of the fmt library the _format() construct was used to compile the format patterns at run time. That's why there are some uses of the literal left in places where performance might be more critical, they need a closer look. See osm2pgsql-dev#1704
We use fmt for formatting strings. Currently version 8.0.1 is vendored in. Newer versions 8.1.0 and 9.0.0 are available, but the syntax
""_format(...)
is deprecated in 8.1 and removed in 9.0. It can be added back with something like this informat.hpp
:But this doesn't seem to work when compiling in C++20 mode. To be more exact, it does work locally for me, but not on Github Actions.
https://github.com/openstreetmap/osm2pgsql/actions/runs/2733557866
Related issues:
On the other hand: In C++20 we should be able to switch to
std::format
. But unless we write a compatibility layer, a solution that works everywhere is way off.See also #1010.
The text was updated successfully, but these errors were encountered: