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>: Incorrect output formatting negative years #3166

Open
blippyblips opened this issue Oct 20, 2022 · 8 comments
Open

<chrono>: Incorrect output formatting negative years #3166

blippyblips opened this issue Oct 20, 2022 · 8 comments
Labels
chrono C++20 chrono LWG issue needed A wording defect that should be submitted to LWG as a new issue

Comments

@blippyblips
Copy link

blippyblips commented Oct 20, 2022

Describe the bug
Incorrect values when formatting negative std::chrono::years with %y (last 2 only).

Command-line test case

C:\Temp2>type repro.cpp
#include <chrono>
#include <format>
#include <iostream>

int main() {
 std::chrono::year y1{-3053};
 std::cout << std::format("{:%y}", y1);
 return 0;
}
C:\Temp2>cl /EHsc /W4 /WX /std:c++20 .\repro.cpp & repro.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.33.31630 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp
Microsoft (R) Incremental Linker Version 14.33.31630.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:repro.exe
repro.obj
47

Expected behavior
Would expect it to print 53

STL version
Microsoft Visual Studio Community 2022 Version 17.3.6 https://github.com/microsoft/STL/tree/main

Additional context
Current code in master:
https://github.com/microsoft/STL/blob/2f8342a3a57fb157d881c6a2d42a917d20e413f8/stl/inc/chrono#L3168

Not sure about the reason for the +100 currently there but would expect something along these lines

_NODISCARD static pair<int, int> _Decompose_year(const int _Year)
{
    int _Two_d_year = _Year % 100;
    return { _Year - _Two_d_year, abs(_Two_d_year) };
}
@MattStephanson
Copy link
Contributor

MattStephanson commented Oct 21, 2022

I think this is the intended behavior. The %C century flag is specified in [tab:time.format.spec] as "[t]he year divided by 100 using floored division." So for -3053, that would give -31, and our convention is that the year is always century_part*100+two_digit_year, with an addition regardless of the sign of the century or year. I think this makes the most sense, given the floored division specification.

@blippyblips
Copy link
Author

blippyblips commented Oct 21, 2022

I agree for %C this is doing the correct thing but this same function is also being used as part of the implementation for %y.
The last two decimal digits of the year (https://eel.is/c++draft/tab:time.format.spec#row-36-column-2-sentence-1).
In that context the pair.second from _Decompose_year is being used and it's only correct for positive inputs.

@MattStephanson
Copy link
Contributor

You're right that the standard says "[t]he last two decimal digits of the year." But in practice this is often interpreted to mean the number of years since the beginning of the century. Obviously these meanings are the same when the year is positive and different when it's negative, so I suspect people sometimes say one thing while meaning another.

The original proposal claims "strftime-like formatting", so we can look to that as a guide for existing practice. You can see at https://godbolt.org/z/Gbz7harWd that the format string "%C %y gives -31 47, which agrees with MSVC STL. On the other hand, {fmtlib}, which is also influential as to existing practice, gives -30 53. This disagrees with the standard as to %C and with strftime as to %y. It also formats the first centuries BCE and CE as -0 and 00, which seems less than ideal for an integer value.

On the whole, there doesn't seems to be a clear consensus for what the result should be, so maybe an LWG issue is needed to clarify.

@blippyblips
Copy link
Author

blippyblips commented Oct 22, 2022

It does sound like there should be separate "years since start of century" "nearest century" "truncated century" and "last two decimals of the year" identifiers, even if they are the same in many cases it's really useful for expressing intent.

C Standard
C++ Time Format for reference

The C17 draft has slightly different phrasing then the eel.is c++ draft but it matches closer with fmts implementation of the formatting identifiers, specifically it calls for the truncated integer after division by 100 instead of floored division for %C and for %y it mentions that it's 'replaced' by the last 2 digits of the year, that seems much more strictly defined.

About the 00 case in FMT, that seems to come from 2 things,

  1. When writing negative numbers they seem to have interpreted the standard saying 4 digits as 4 characters and they remove 1 when adding the - sign, otherwise it would also have written -00.
  2. %Y for positive (0 included) is written in terms of %C%y so you're getting one zero from the century and another from the "decade/unit years"

Year formatting in fmt for reference

It makes more sense when you read the 2 different versions of the time specs(c vs c++) and try to combine them.

@StephanTLavavej StephanTLavavej added the LWG issue needed A wording defect that should be submitted to LWG as a new issue label Oct 26, 2022
@StephanTLavavej
Copy link
Member

We talked about this the weekly maintainer meeting and agree that an LWG issue seems necessary here. @MattStephanson would you like to file an issue, or would you like someone else to look into drafting one?

@StephanTLavavej StephanTLavavej added the chrono C++20 chrono label Oct 26, 2022
@MattStephanson
Copy link
Contributor

@StephanTLavavej --- I'll write it up and see if Victor or Howard want to add anything before it's submitted.

@MattStephanson
Copy link
Contributor

Update for anyone who's watching this issue: I sent out the draft issue yesterday. Waiting to see if one more person wants to comment, but planning to send to LWG by next Monday at the latest.

@MattStephanson
Copy link
Contributor

LWG-3831

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrono C++20 chrono LWG issue needed A wording defect that should be submitted to LWG as a new issue
Projects
None yet
Development

No branches or pull requests

3 participants