Implement precision for floating-point durations. #1012
Conversation
vitaut
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks great, just a few minor comments.
include/fmt/chrono.h
Outdated
| struct formatter<std::chrono::duration<Rep, Period>, Char> { | ||
| private: | ||
| align_spec spec; | ||
| core_format_specs pspec; |
There was a problem hiding this comment.
We only need int precision instead of the whole core_format_specs here.
There was a problem hiding this comment.
That was my original implementation. I switched over to core_format_specs because of the has-precision predicate defined there.
include/fmt/chrono.h
Outdated
| core_format_specs pspec; | ||
| typedef internal::arg_ref<Char> arg_ref_type; | ||
| arg_ref_type width_ref; | ||
| internal::arg_ref<Char> precision_ref; |
There was a problem hiding this comment.
internal::arg_ref<Char> -> arg_ref_type
include/fmt/chrono.h
Outdated
| const auto arg = internal::make_arg<context>(val); | ||
| if (!visit_format_arg(cf, arg)) | ||
| visit_format_arg(af, arg); | ||
| return ctx.out(); |
There was a problem hiding this comment.
I think this can be significantly simplified by using format_to(out, "{:.{}f}", val, precision) or something like that.
There was a problem hiding this comment.
Sure, but with my performance-hat on I tried to avoid reparsing.
include/fmt/chrono.h
Outdated
| if (begin == end) return begin; | ||
| begin = internal::parse_width(begin, end, handler); | ||
| if (begin == end) return begin; | ||
| begin = parse_precision(begin, end, handler, std::is_floating_point<Rep>{}); |
There was a problem hiding this comment.
Do we actually need tag dispatched parse_precision? Wouldn't if (std::is_floating_point<Rep>{}) work and be much simpler?
There was a problem hiding this comment.
We don't need it. The tag dispatch is just a clear hint to the compiler to not even try instantiating dead code.
| EXPECT_EQ(" 1.2ms ", fmt::format("{:^{}.{}}", dms(1.234), 7, 1)); | ||
| EXPECT_EQ(" 1.23ms ", fmt::format("{0:^{2}.{1}}", dms(1.234), 2, 8)); | ||
| EXPECT_EQ("=1.234ms=", fmt::format("{:=^{}.{}}", dms(1.234), 9, 3)); | ||
| EXPECT_EQ("*1.2340ms*", fmt::format("{:*^10.4}", dms(1.234), 10, 4)); |
0aaffc7 to
b0c7c6a
Compare
|
BTW, what about other |
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004. Signed-off-by: Daniela Engert <dani@ngrt.de>
b0c7c6a to
245a3f5
Compare
|
Merged, thanks!
I don't have any plans regarding this at the moment. The whole exercise was mainly to support the chrono integration proposal. |
The formatting syntax follows p1361r0, augmented by a precision field as proposed in #1004.
Signed-off-by: Daniela Engert dani@ngrt.de