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

<chrono>: [time.duration.io] output #1341

Merged

Conversation

MattStephanson
Copy link
Contributor

Partially implements #12, specifically operator<<(duration).

I've implemented it with "µs" for the wchar_t overload, but I'm not certain that's a good idea. It's clever, but there's value in encoding-independent output as well. Comments welcome.

@MattStephanson MattStephanson requested a review from a team as a code owner October 5, 2020 05:15
@MikeGitb
Copy link

MikeGitb commented Oct 5, 2020

If this isn't specified in the standard, I think consistency is more important than being "fancy". Are there any common windows (or linux) command line tools (or logging formats) that use "µs"? If they don't use it, I don't think the standard library needs to innovate here.

Anyone knows what libstdc++ or hinnants date library are doing?

@SuperWig
Copy link
Contributor

SuperWig commented Oct 5, 2020

If this isn't specified in the standard

It's implementation-defined https://eel.is/c++draft/time.duration.io#1.5

@AdamBucior
Copy link
Contributor

Personally I prefer "µs" because it's more correct. I don't see much value in iostreams being consistent with outputs across encodings but I see value in correctness and disambiguity.

@MikeGitb
Copy link

MikeGitb commented Oct 5, 2020

I don't see much value in iostreams being consistent with outputs across encodings

Because for example you now also have to handle both cases in de-serialization, or search operations/regex expressions. "µs" may be more correct, but I don't see any practical gain/advantage over "us" and virtually any entity that "looks" at the output (be it a human or a program" will have to be able to handle the "us" case anyway.

@MikeGitb
Copy link

MikeGitb commented Oct 5, 2020

Just to be clear though: I don't have a strong opinion on the topic, nor nor lots of experience to backup the opinion.

Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

This looks like it's in pretty good shape to me! Thanks for tackling the first of the formatting chrono things 😸
Mostly nitpicks and an ask for more testing!

stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej 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 implementing this streaming operator! The code and tests look good, I just found some easily fixable issues. I'll validate a fix (double-checking the _ITERATOR_DEBUG_LEVEL stuff) and will push changes soon.

stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
stl/inc/chrono Outdated Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

FYI @mnatsuhara, I pushed changes after you approved.

I was able to minimize the clang-format off region around the _Suffixes table. clang-format recognizes trailing commas and will place initializers on separate lines. However, clang-format doesn't handle pair{A, B} CTAD very nicely (it formats the braces as if they're opening a code block), so the macro is now guarded.

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020
@StephanTLavavej StephanTLavavej self-assigned this Nov 5, 2020
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020
@StephanTLavavej
Copy link
Member

FYI @mnatsuhara, I pushed significant changes after you approved. In our internal test suite, the compiler's object file size test failed. This includes most STL headers, does nothing with them, and verifies (with dumpbin) that the object file doesn't contain unnecessary data. It was failing because of the lookup table in this PR. I attempted to refactor it as inline constexpr variable templates, but that emitted the same read-only data.

I eventually found a solution. Now, the table is encoded as if constexpr within a function _Get_literal_unit_suffix(). This returns the desired string (or nullptr) immediately if called at runtime (in fact, it is always evaluated at compile-time right now, because it's used to initialize constexpr auto _Suffix). This avoids emitting any read-only data into the object file, if the function isn't instantiated.

Although it's a mechanically large change, the actual structure of @MattStephanson's code is essentially unchanged (the callsites just change what they're calling). Because this eliminated the table, there was no reason to retain the class _Units_suffix_helper, so I transformed its static member function _Get_general into a free function _Get_general_unit_suffix (hiding whitespace changes makes this clearer).

Upon reviewing my own code, it occurs to me that this strategy, and the original strategy, doesn't handle character types other than char and wchar_t, whereas the Standard seems to imply that narrow strings should be inserted for other character types. As this is not a regression I think we can move forward with the PR and fix anything later.

@MattStephanson
Copy link
Contributor Author

doesn't handle character types other than char and wchar_t

That shouldn't be too much trouble to fix, unless you don't want to disturb your current batch merge. I think I can just switch the order of the if/else to make char the fallback, then use a conditional to get the correct return type. Or just always return the narrow type, since any basic_ostream has to accept a const char* (which I just learned today). So it's really just an optimization to return a pre-widened const wchar_t*.

@StephanTLavavej
Copy link
Member

That shouldn't be too much trouble to fix, unless you don't want to disturb your current batch merge.

Definitely don't want to disturb it. A followup PR will be great!

I think I can just switch the order of the if/else to make char the fallback, then use a conditional to get the correct return type.

I think that would be best.

@StephanTLavavej StephanTLavavej merged commit 6458199 into microsoft:master Nov 6, 2020
@StephanTLavavej
Copy link
Member

Thanks again for implementing this streaming operator - programmers will enjoy it for many gigaseconds. ⌚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants