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.

Sign up for GitHub

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

Jump to bottom

allow %x for formatting year_month_day #2762

Merged
merged 2 commits into from Jun 16, 2022
Merged

allow %x for formatting year_month_day #2762

merged 2 commits into from Jun 16, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jun 5, 2022

Fixes #2761

I'm not sure if it's correct.

C++ standard is unclear: https://eel.is/c++draft/time.format#6

But cppreference says that %x is supported for year_month_day, year_month_day_last, year_month_weekday and year_month_weekday_last

@fsb4000 fsb4000 requested a review from a team as a code owner June 5, 2022 18:05
@CaseyCarter CaseyCarter added bug Something isn't working format C++20/23 format labels Jun 5, 2022
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Jun 5, 2022
@StephanTLavavej StephanTLavavej added the chrono C++20 chrono label Jun 6, 2022
@StephanTLavavej
Copy link
Member

I believe that this is correct (although the Standard is unusually elliptical in this area, these types do provide the necessary information).

@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Jun 7, 2022
@barcharcraz barcharcraz added this to @barcharcraz in Maintainer Priorities Jun 14, 2022
barcharcraz
barcharcraz approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@barcharcraz barcharcraz 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 good. Ideally, we would test the locale specific orderings, but with current test coverage this looks like a strict improvement, even if the locale stuff were to be broken so I don't feel it needs those tests to be merged.

@barcharcraz barcharcraz moved this from Final Review to Ready To Merge in Code Reviews Jun 15, 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 8fca26f into microsoft:main Jun 16, 2022
Code Reviews automation moved this from Ready To Merge to Done Jun 16, 2022
Maintainer Priorities automation moved this from @barcharcraz to Done Jun 16, 2022
@StephanTLavavej
Copy link
Member

Thanks for fixing this chronat bug! 📅 ✏️ 🎉

@fsb4000 fsb4000 deleted the fix2761 branch June 16, 2022 04:01
fsb4000 added a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@fsb4000 @StephanTLavavej
allow %x for formatting year_month_day (microsoft#2762)
e6d2846 
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barcharcraz barcharcraz barcharcraz approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working chrono C++20 chrono format C++20/23 format
Projects
Maintainer Priorities
Done
Code Reviews
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<chrono>: formatter<chrono::year_month_day> is not fully implemented.
4 participants
@fsb4000 @StephanTLavavej @barcharcraz @CaseyCarter