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

Fix chrono printing #2814

Merged
merged 10 commits into from
Jul 11, 2022
Merged

Conversation

barcharcraz
Copy link
Member

@barcharcraz barcharcraz commented Jun 22, 2022

There may be more cases of this behavior that are not fixed here, but this should improve things for basic formatting with {}.

There's a bunch of standardize that appears unfortunate when considering types that "underflow" in this manner, the wording tries to make sure you don't truncate when converting from large periods to tiny ones, but the reverse is also prevented, even though it's not a big deal and is similar to the "rounding" that already happens when converting from say, seconds, to days with duration_cast.

For example, chrono::ceil and chrono::floor makes sense in this case.

Fixes VSO-1521926/AB#1521926.

@barcharcraz barcharcraz requested a review from a team as a code owner June 22, 2022 01:53
{} is specified to be equivalent to formatting
using iostreams <<, but for very small durations
we would fail to compile such code because of conversions
to longer durations failing (since we form the hh_mm_ss structure
for all formats, even though in this case it isn't used.

This change should make some of the specifiers that _do_ use that
structure work as well (for example {:%H}, attoseconds).

This problem occurs for any chrono conversion where the source type
can not represent even one period of the target type.
@StephanTLavavej StephanTLavavej added bug Something isn't working format C++20/23 format chrono C++20 chrono labels Jun 22, 2022
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
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
@StephanTLavavej

This comment was marked as resolved.

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

Thanks, this looks good! I verified that the underflow check is correctly using the GCDs to reduce. I validated and pushed a conflict-free merge with main followed by changes for a couple of minor comments that hadn't been addressed (qualification, parameter order).

@StephanTLavavej StephanTLavavej removed their assignment Jul 7, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jul 9, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 51c24c4 into microsoft:main Jul 11, 2022
@StephanTLavavej
Copy link
Member

Thanks for fixing these user-reported compiler errors! ⏱️ 💥 😺

fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chrono C++20 chrono format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
  翻译: