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

Formatting chrono durations and time_points #864

Closed
mwinterb opened this issue Sep 16, 2018 · 8 comments
Closed

Formatting chrono durations and time_points #864

mwinterb opened this issue Sep 16, 2018 · 8 comments

Comments

@mwinterb
Copy link
Contributor

I have a formatter for the std::chrono::duration template based on what's proposed for C++20's operator<<. The main differenceis operator<< is specified as using std::to_string for the numeric part, right now I'm just using the default formatter for the representation type (for floating point types, to_string behaves like %f, while fmt behaves like %g).
However, I like much of what is available in to_stream. But there are some parts I'd probably not include, as I can't decide what an AM/PM designation or any of the other 12-hour clock-centric conversion specifiers mean for a duration. For instance, "my uptime is 01:03pm" seems nonsensical. I'd probably also want to add %f for subsecond values as suggested for time_points in #743.

  • Does including a formatter that just does the basics of operator<< now constrain future changes?
    • Is there existing precedent for formatting types in such a drastically different way based on the format string? (I think "does the format specifier include a %? yes: behave like to_stream. no: format as a number + unit suffix" is likely an adequate differentiator).
    • Would adding to_stream-like support limit compile time checks in the basic case? fmt::format(FMT_STRING("{:I have been running for %H hours and %M minutes}"), dur); would be legal, so the formatter would need to support collecting an "unbounded" amount of characters in parse, like the std::tm formatter, which seems limiting. Or can parse merely store the locations of the beginning and ending of the format specifier instead? I have been assuming that the lifetime of the format string was possibly over by the time format is called and that is why the tm formatter collected the format string in parse, but maybe that's inaccurate?
@vitaut
Copy link
Contributor

vitaut commented Sep 19, 2018

Does including a formatter that just does the basics of operator<< now constrain future changes?

It doesn't, although we'd want the operator<<'s output be consistent with to_stream's default if there is one. What does to_stream(os, "", d) write?

Is there existing precedent for formatting types in such a drastically different way based on the format string?

Kind of. bools are formatted as true or false if format specifiers are empty.

Would adding to_stream-like support limit compile time checks in the basic case?

No, it wouldn't.

Or can parse merely store the locations of the beginning and ending of the format specifier instead?

The parser doesn't store the string itself, it just iterates over it and invokes parser actions as it goes.

I have been assuming that the lifetime of the format string was possibly over by the time format is called and that is why the tm formatter collected the format string in parse, but maybe that's inaccurate?

That's inaccurate. The format string is guaranteed to exist throughout the parsing and formatting phases.

@mwinterb
Copy link
Contributor Author

It doesn't, although we'd want the operator<<'s output be consistent with to_stream's default if there is one. What does to_stream(os, "", d) write?

From the description, and reading Howard's reference implementation, it seems like it would write an empty string. It's very much like strftime in that way. This seems like a poor default when there is a sane one, especially if one views durations strictly as a sugared number.

The parser doesn't store the string itself, it just iterates over it and invokes parser actions as it goes.

I was thinking specifically of the formatter for std::tm, which does store the format string:

auto parse(ParseContext &ctx) -> decltype(ctx.begin()) {
... but, since it directly invokes strftime which needs a nul terminated string, it makes sense that it can't just store begin and end iterators.

That's inaccurate. The format string is guaranteed to exist throughout the parsing and formatting phases.

Okay, good. Thanks for the correction.

Just to put my idea in concrete terms, this is what I think would be convenient:

const std::chrono::nanoseconds ns(123456);
fmt::print("{}\n", ns);
// outputs
// 123456ns
// This is what happens if "we" do nothing, C++20 ships with the currently specified operator<<, and fmt/ostream.h is included.

fmt::print("{:n}\n", ns);
// outputs
// 123,456ns

const std::chrono::duration<double> floating_seconds(0.12345);
fmt::print("{:.3f}\n", floating_seconds);
// outputs
// 0.123s

const std::chrono::minutes mins(90);
fmt::print("{}\n", mins);
// outputs
// 90min
// again, this is what will "magically" happen

fmt::print("{:%H:%M}\n", mins);
// outputs
// 01:30

const std::chrono::minutes mins2 = std::chrono::hours(100) + std::chrono::minutes(16);
fmt::print("{:%H:%M}\n", mins2);
// I think this would output:
// 100:16
// but I haven't gotten that far in investigating to_stream's behavior in this instance.

// assuming C++20 ships, the following two lines would produce identical output.
std::cout << ns << '\n';
fmt::print("{}\n", ns);

What I presently have covers the first 4 examples as that was my immediate need, in addition to being a lot simpler to cover. The 5th seems convenient (and, indeed, I found one spot in my codebase that wants effectively this), but I didn't know if it was too odd to mix and match both styles, or if there should be a distinct type that would support the to_stream syntax, but normal durations would output as the number followed by the unit suffix.

@vitaut
Copy link
Contributor

vitaut commented Sep 20, 2018

This seems like a poor default when there is a sane one, especially if one views durations strictly as a sugared number.

Yes, this is a horrible default. I think we should follow the Python's approach and pick something actually useful here because it's gonna be used often:

>>> '{}'.format(datetime.date(2002, 3, 11))
'2002-03-11'
>>> '{:%Y}'.format(datetime.date(2002, 3, 11))
'2002'

All your examples look very reasonable to me. PRs are welcome =).

@mwinterb
Copy link
Contributor Author

Sorry -- I thought I had something reasonable, but then realized I'm probably excluding too many platforms since I was relying on FMS_HAS_INCLUDE which isn't supported on all platforms that support chrono, but not every standard library that fmt supports has chrono. Minor thing, I just need to fix it up. And the implementation may not work with all output iterators.
But I also broke something at work, and since I'm going to be gone for a week, I need to get that fixed first. So, hopefully in October I'll have a PR for this.

Also, for the tests, I cheated and based everything on https://github.com/HowardHinnant/date/blob/master/test/date_test/durations_output.pass.cpp. Is that okay to bring that copyright in, or should I work on a new set of tests?

@vitaut
Copy link
Contributor

vitaut commented Sep 21, 2018

No worries, take your time. It's OK to bring tests from Howard's date repo, just make sure to keep their original license & copyright notice in the test source file(s).

@mwinterb mwinterb changed the title Formatting chrono durations Formatting chrono durations and time_points Nov 27, 2018
@mwinterb
Copy link
Contributor Author

Sorry. Sorry.

@vitaut
Copy link
Contributor

vitaut commented Nov 29, 2018

FWIW, I started experimenting with this (https://github.com/fmtlib/fmt/blob/master/include/fmt/time.h#L138) mostly to convince myself that chrono formatting is implementable using the current extension API for the forthcoming chrono-fmt ISO C++ paper.

@vitaut
Copy link
Contributor

vitaut commented Dec 10, 2018

An experimental chrono duration formatter is now available at https://github.com/fmtlib/fmt/blob/master/include/fmt/chrono.h . It should be fully functional but not optimized. Improvements are welcome.

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