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

fmtlib and C++20 #1704

Closed
joto opened this issue Jul 26, 2022 · 4 comments
Closed

fmtlib and C++20 #1704

joto opened this issue Jul 26, 2022 · 4 comments

Comments

@joto
Copy link
Collaborator

joto commented Jul 26, 2022

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 in format.hpp:

inline constexpr auto operator"" _format(const char *s, std::size_t n)
{
    return [=](auto &&...args) {
#if FMT_VERSION < 80100
        return fmt::format(std::string_view{s, n}, args...);
#else
        return fmt::format(fmt::runtime(std::string_view{s, n}), args...);
#endif
    };
}

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.

@joto
Copy link
Collaborator Author

joto commented Jul 26, 2022

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.

@mmd-osm
Copy link
Contributor

mmd-osm commented Jul 30, 2022

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?

@joto
Copy link
Collaborator Author

joto commented Jul 30, 2022

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.

@joto
Copy link
Collaborator Author

joto commented Jan 4, 2023

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 fmt::format(). In some places that are more critical for performance, we might need special solutions.

joto added a commit to joto/osm2pgsql that referenced this issue Jan 6, 2023
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
@lonvia lonvia closed this as completed in c8f4af0 Jan 15, 2023
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

No branches or pull requests

2 participants