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

Implement P2419R2 Clarify Handling Of Encodings In Localized Formatting Of chrono Types #2977

Merged
merged 5 commits into from
Aug 22, 2022

Conversation

cpplearner
Copy link
Contributor

Fixes #2927.

When the literal encoding is UTF-8, This ensures that the strings produced by chrono formatters are always encoded in UTF-8, by first formatting with wchar_t, then converting the output to UTF-8. (Is there a better way?)

I didn't convert the output of _Custom_write, because it seems that _Custom_write generally does not deal with localized formatting.

@cpplearner cpplearner requested a review from a team as a code owner July 31, 2022 18:06
@cpplearner cpplearner force-pushed the p2419 branch 2 times, most recently from d1fb832 to ba73f5a Compare July 31, 2022 23:50
@CaseyCarter CaseyCarter added format C++20/23 format cxx23 C++23 feature labels Aug 1, 2022
@strega-nil-ms strega-nil-ms self-assigned this Aug 2, 2022
@barcharcraz
Copy link
Member

does this need a __cpp_lib_format bump?

stl/inc/chrono Outdated
@@ -5351,6 +5418,24 @@ namespace chrono {

_Validate_specifiers(_Spec, _Val);

#if defined(_MSVC_EXECUTION_CHARACTER_SET) && _MSVC_EXECUTION_CHARACTER_SET == 65001
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be marked with the same transition comment as _Is_execution_charset_self_synchronizing in <format> to remove the
ifdef check once EDG supports this feature.

stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
@cpplearner
Copy link
Contributor Author

does this need a __cpp_lib_format bump?

P2508R1 (Expose std::basic-format-string) updates the same macro. I'll wait until that paper is implemented.

stl/inc/filesystem Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
@strega-nil-ms strega-nil-ms removed their assignment Aug 4, 2022
@StephanTLavavej
Copy link
Member

@cpplearner Just checking on the status of this PR - are you planning to make changes here, or should we mark it as blocked until there's a PR for #2937?

@cpplearner
Copy link
Contributor Author

I thought I made the changes but I forgot to push them 😢

@StephanTLavavej
Copy link
Member

@cpplearner Checking again - are your changes ready to push? @barcharcraz and @strega-nil-ms would like to review them 😸

@cpplearner
Copy link
Contributor Author

I've pushed db93009 after posting #2977 (comment) (GitHub UI is weird). I think this PR is ready for review now.

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 now looks good.

I'm still a tiny bit annoyed with the approach of narrowing the wide versions of the locale data, but I can't think of a better way to do it. Maybe I'll make an issue to switch to the UTF-8 locale data once we no longer support systems that lack it.

@strega-nil-ms strega-nil-ms self-assigned this Aug 19, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, although I'm kinda sad for using wostringstream.

@strega-nil-ms strega-nil-ms removed their assignment Aug 19, 2022
@StephanTLavavej
Copy link
Member

LGTM too, thanks @cpplearner! FYI @barcharcraz @strega-nil-ms I pushed a one-line change after you approved, moving the yvals_core.h comment to the C++20 section as this is specific to chronat.

@StephanTLavavej StephanTLavavej self-assigned this Aug 20, 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 ff29e7a into microsoft:main Aug 22, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this feature - just in time for 17.4 Preview 3! 😹 ⏱️ 🎉

@cpplearner cpplearner deleted the p2419 branch August 25, 2022 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2419R2 Clarify Handling Of Encodings In Localized Formatting Of chrono Types
5 participants