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

std::chrono duration formatting is lacking floating point specifiers #1004

Closed
DanielaE opened this issue Jan 11, 2019 · 18 comments
Closed

std::chrono duration formatting is lacking floating point specifiers #1004

DanielaE opened this issue Jan 11, 2019 · 18 comments

Comments

@DanielaE
Copy link
Contributor

Consider this test code:

typedef std::chrono::duration<float, std::milli> fms;
EXPECT_EQ("1.234ms", fmt::format("{}", fms(1.234)));
typedef std::chrono::duration<double, std::milli> dms;
EXPECT_EQ("1.234ms", fmt::format("{}", dms(1.234)));
EXPECT_EQ("1.2ms", fmt::format("{:.2}", dms(1.234)));

The first and second test passes, i.e. floating point representation types in std::chrono::duration work in general. The last fails by returning .2 rather than formatting the given time quantity of 1.2345 ms to the given precision. There is seemingly no application of floating point specifiers possible.

@DanielaE
Copy link
Contributor Author

I've implemented some stuff to handle the precision specification, but this is probably a dead end.
DanielaE@1cec292

@HowardHinnant
Copy link

HowardHinnant commented Jan 12, 2019

Here is one way to address this:

using secondsm4 = std::chrono::duration<long, std::ratio<1, 10'000>>;
fmt::format("{}", dms{floor<secondsm4>(dms{1.234})});  // 1.2ms

@DanielaE
Copy link
Contributor Author

Right, in this case (static precision, static formatting string), the workaround of shifting the precision part of the formatting process onto the user certainly works. In the general case it doesn't and maintenance in non-trivial codebases with the prospect of engineering changes might become a nightmare. After all, {fmt}'s added value and most noble goal is relieving users from the burden of formatting.
I have no problem if chrono's output stream operator doesn't handle this (at least, this is my conclusion after reading it's specification). It only means that {fmt} should take this into consideration and possibly not rely on chrono's facilities in all formatting scenarios.

@vitaut
Copy link
Contributor

vitaut commented Jan 13, 2019

Supporting precision for duration types with floating-point representation makes a lot of sense to me.

this is probably a dead end

@DanielaE why do you think so?

@DanielaE
Copy link
Contributor Author

Well, I am not sure about the direction of the implementation of this feature. I am glad that you support the idea. My experimental stuff does a consistent parsing so far, but it's totally unclear to me on how to proceed from there. I've learned from Howards comment that backing the formatting process on chrono's output streaming operator is not the way to go in this case. Currently I envision an implementation which checks the formatting string for chrono formatting specifiers and defers to chrono-based formatting in this case with exactly it's features and limitations. If there are none, {fmt}'s facilities are superior and implementing chrono::duration formatting is trivial as I've shown in my Meeting C++ 2018 presentation. Taking such a route makes my experiment obsolete: therefore the 'dead end'.

@vitaut
Copy link
Contributor

vitaut commented Jan 16, 2019

p1361r0 proposes support for fill, width, and alignment before the main strftime-like specifiers:

format-spec     ::= [[fill] align] [width] [conversion-spec [chrono-specs]]
chrono-specs    ::= chrono-spec [chrono-specs]
chrono-spec     ::= literal-char | conversion-spec
literal-char    ::= <a character other than '{' or '}'>
conversion-spec ::= '%' [modifier] type

We could easily incorporate precision there:

format-spec     ::= [[fill] align] [width] ['.' precision] [conversion-spec [chrono-specs]]

@DanielaE
Copy link
Contributor Author

This makes lots of sense to me. I like it a lot.

I've been studying the current state of chrono formatting and I'll probably revoke the 'dead end' assessment of my experimental code. So I will go and try to implement the full precision functionality - hopefully during the upcoming weekend.

DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 18, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 18, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 18, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE
Copy link
Contributor Author

@vitaut Any opinion on DanielaE@f859515 before I open a PR? In this implementation, the precision applies to the numeric formatting in absence of [conversion-spec [chrono-specs]].

DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE
Copy link
Contributor Author

DanielaE@7571832 is a bit more refined with some preparation for wchar_t-formatting (or other character types).

@vitaut
Copy link
Contributor

vitaut commented Jan 19, 2019

Looks good to me. Please open a PR and I'll comment there with some minor suggestions.

DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE
Copy link
Contributor Author

done

DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 19, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 21, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 22, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 22, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in fmtlib#1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
vitaut pushed a commit that referenced this issue Jan 23, 2019
The formatting syntax follows p1361r0, augmented by a precision field as proposed in #1004.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE
Copy link
Contributor Author

DanielaE commented Jan 23, 2019

Expect a follow-up to keep track with Howard's date which added %q and %Q as new formatting specifications.

@DanielaE
Copy link
Contributor Author

@HowardHinnant, where would you put the precision specifier into the %q field? Something like %.3q (static precision) and %.{}q (dynamic precision)? Or something easier to parse?

@HowardHinnant
Copy link

I'm really not sure, but am guessing something like "{:.3%Q %q}". The .3 would modify the %Q (the value), but not %q (the units).

@DanielaE
Copy link
Contributor Author

Thinking about it, this is a bit underspecified so far imho in case of multiple %Q fields. Therefore I propose to define that there is at most one single common precision specifier allowed, e.g. {:.{} SI requires '%Q %q' rather than '%Q%q'}. In other words: there is at most one set of formatting specifiers which affects the rendering of all fields within the same replacement field in the same way.

@vitaut
Copy link
Contributor

vitaut commented Jan 23, 2019

there is at most one set of formatting specifiers which affects the rendering of all fields within the same replacement field in the same way.

Seems reasonable.

DanielaE added a commit to DanielaE/fmt that referenced this issue Jan 25, 2019
Howard Hinnant's 'date' library recently gained these two new formatting specifiers. This implementation in {fmt} includes support for 'std::chrono::duration' specializations with floating-point representation types and user-definable precision.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE
Copy link
Contributor Author

Is implemented now - enjoy 😄

@vitaut
Copy link
Contributor

vitaut commented Jan 27, 2019

I guess this issue can be closed now.

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

3 participants