-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add formatter for std::atomic #3574
Conversation
You need to include |
Fixed and passed all tests |
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.
Thanks for the PR.
include/fmt/std.h
Outdated
FMT_EXPORT | ||
template <typename Char> | ||
struct formatter<std::errc, Char> | ||
: formatter<std::underlying_type<std::errc>::type, Char> { | ||
template <typename FormatContext> | ||
FMT_CONSTEXPR auto format(std::errc v, FormatContext& ctx) const | ||
-> decltype(ctx.out()) { | ||
using underlying_type = std::underlying_type<std::errc>::type; | ||
return formatter<underlying_type, Char>::format( | ||
static_cast<underlying_type>(v), ctx); | ||
} | ||
}; |
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.
I am not sure if providing a formatter for errc is a good idea. I think it should be up to the user to decide how they want to present it and if they want a number they could use to_underlying.
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 there any way to provide a default formatter if no other formatter exists?
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.
Considering that it is easy to format errc
as a number with to_underlying
, I don't think it's worth exploring the default formatter option.
Thank you! |
BTW could you add std::atomic to the list of formattable types in https://github.com/fmtlib/fmt/blob/master/doc/api.rst#standard-library-types-formatting? |
template <typename FormatContext> | ||
auto format(const std::atomic<T>& v, FormatContext& ctx) const | ||
-> decltype(ctx.out()) { | ||
return formatter<T, Char>::format(v.load(), ctx); |
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.
Maybe its better to use load(std::memory_order_acquire) or even load(std::memory_order_relaxed)?
There is no formatter for
std::atomic
andstd:errc
and injectformat_as
instd
namespace is not a good idea.This patch implement formatter for above types.