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

Implement precision for floating-point durations. #1012

Merged

Conversation

DanielaE
Copy link
Contributor

The formatting syntax follows p1361r0, augmented by a precision field as proposed in #1004.

Signed-off-by: Daniela Engert dani@ngrt.de

Copy link
Contributor

@vitaut vitaut left a 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! Looks great, just a few minor comments.

@@ -362,8 +362,10 @@ template <typename Rep, typename Period, typename Char>
struct formatter<std::chrono::duration<Rep, Period>, Char> {
private:
align_spec spec;
core_format_specs pspec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need int precision instead of the whole core_format_specs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my original implementation. I switched over to core_format_specs because of the has-precision predicate defined there.

typedef internal::arg_ref<Char> arg_ref_type;
arg_ref_type width_ref;
internal::arg_ref<Char> precision_ref;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal::arg_ref<Char> -> arg_ref_type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh ...

const auto arg = internal::make_arg<context>(val);
if (!visit_format_arg(cf, arg))
visit_format_arg(af, arg);
return ctx.out();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be significantly simplified by using format_to(out, "{:.{}f}", val, precision) or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but with my performance-hat on I tried to avoid reparsing.

@@ -408,6 +460,9 @@ struct formatter<std::chrono::duration<Rep, Period>, Char> {
begin = internal::parse_align(begin, end, handler);
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>{});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need tag dispatched parse_precision? Wouldn't if (std::is_floating_point<Rep>{}) work and be much simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DanielaE DanielaE force-pushed the feature/chrono_duration-add-precision branch 2 times, most recently from 0aaffc7 to b0c7c6a Compare January 19, 2019 17:58
@DanielaE
Copy link
Contributor Author

BTW, what about other wchar_t and other character types - any plans on that? All these string literals may become annoying then.

The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE DanielaE force-pushed the feature/chrono_duration-add-precision branch from b0c7c6a to 245a3f5 Compare January 22, 2019 06:19
@vitaut vitaut merged commit 9f70b03 into fmtlib:master Jan 23, 2019
@vitaut
Copy link
Contributor

vitaut commented Jan 23, 2019

Merged, thanks!

BTW, what about other wchar_t and other character types - any plans on that?

I don't have any plans regarding this at the moment. The whole exercise was mainly to support the chrono integration proposal.

@DanielaE DanielaE deleted the feature/chrono_duration-add-precision branch January 23, 2019 15:52
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

Successfully merging this pull request may close these issues.

2 participants