treewide: use fmtlib's fmt::format_string in format() #1727
Conversation
|
now scylla compiles with the change. but still, this change is a breaking change. |
EDIT, this is how ADL works. in other words, if the argument(s) are in the "std" namespace, so we would have to explicitly call |
this change replace all `format()` with `seastar::format()`. this was not necessary if we don't change the function signature of `seastar::format()`, even if we compile the tree with libstdc++ shipped with GCC-13, which implements `std::format()`. because `std::format()` accepts a templated parameter of `fmt`, so the `seastar::fmt()` which accepts a plain `const char*` always win if we just call `format()` in the opened namespace of `seastar`. but since we plan to change `seastar::format()` from `format(const char* fmt, A&&... a)` to `format(fmt::format_string<A...> fmt, A&&... a)`, we cannot gurantee that `seastar::format()` will win in the function resolution in the use case above. so we have to change all call sites of this function to `seastar::format()` to disambiguate it. Signed-off-by: Kefu Chai <[email protected]>
because rpc logger needs to talk to both seastar::logger and the user customized logger. the former does not accept `fmt::format_string` yet, while the latter needs to accept `seastar::format()`ed sstring. and we plan to change `seastar::format()` to use compile-time format check. so to cater the needs of both branches. we just decouple rpc logger from `seastar::format()`. and let rpc logger to use `fmt::format_to()` directly. this would allow us to change `seastar::format()`. Signed-off-by: Kefu Chai <[email protected]>
using `fmt::format_string` would allow us to perform compile-time format checking. so in this change, we switch to `fmt::format_string` when fmtlib's version is greater or equal to 8.0, which introduced the compile-time checking. also, because the `fmt::format_string` can only be instantiated with a compile-time constexpr, the fmt string specified in test is changed to a constexpr accordingly. Signed-off-by: Kefu Chai <[email protected]>
|
@scylladb/seastar-maint hello maintainers, could you help review this change? if we have concerns on the backward compatibility like we did in 228bf07, i will add a macro which plays a similar role of |
|
Most reasonable applications don't use |
| @@ -140,9 +140,9 @@ log(A&&... a) { | |||
| */ | |||
| template <typename... A> | |||
| sstring | |||
| format(const char* fmt, A&&... a) { | |||
| format(fmt::format_string<A...> fmt, A&&... a) { | |||
There was a problem hiding this comment.
Do we need another overload for fmt::runtime, or will this work out of the box?
There was a problem hiding this comment.
Ah, runtime_format_string is convertible to basic_format_string.
this series enables
seastar::format()to use compile-time format check.using
fmt::format_stringwould allow us to perform compile-timeformat checking. so in this change, we switch to
fmt::format_stringwhen fmtlib's version is greater or equal to 8.0, which introduced
the compile-time checking.
also, because the
fmt::format_stringcan only be instantiated witha compile-time constexpr, the fmt string specified in test is changed
to a constexpr accordingly.
please note, this is a breaking change which might require caller to use
seastar::format()if the tree is compiled with libstdc++ 13 or a standardlibrary with
std::formt()implemented.