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_test.locale on libc++ #2349

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Fix chrono_test.locale on libc++ #2349

merged 3 commits into from
Jun 14, 2021

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jun 10, 2021

Partial fix for issue #2337 (comment) on libc++.
locale: ja_JP.utf8

See: https://datatracker.ietf.org/doc/html/rfc3629

@@ -298,7 +298,7 @@ inline auto do_write(const std::tm& time, const std::locale& loc, char format,
auto str = os.str();
if (!detail::is_utf8() || loc == std::locale::classic()) return str;
// char16_t and char32_t codecvts are broken in MSVC (linkage errors).
using code_unit = conditional_t<FMT_MSC_VER != 0, wchar_t, char16_t>;
using code_unit = conditional_t<FMT_MSC_VER != 0, wchar_t, char32_t>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why char32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ja_JP.utf8 "chars" may don't included in utf-16 without surrogate pairs.

Today I will update the PR with one more fix and tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If apply commit 6ae63a1 to current master next tests do failed: chrono-test, unicode-test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be char16_t + handling of surrogate pairs for compatibility with msvc where wchar_t is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gcc-4 does not support char16_t/char32_t conversion. In gcc/clang wchar_t is are 4 bytes (utf-32).
For compatibility with gcc 4 we need three implementations:

  • for UTF-16 wchar_t (Windows)
  • for UTF-32 wchar_t (gcc-4)
  • for UTF-?? char16/32_t (gcc-5+, clang).

I think it is better to choose UTF-32 for gcc/clang.

I will try to add support unicode surrogate pairs for windows, but I don't know how to test it.

Copy link
Contributor

@vitaut vitaut Jun 11, 2021

Choose a reason for hiding this comment

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

char16_t being unsupported on some platforms is fine because this code is mostly to handle legacy encodings on Windows. In the unsupported case we should just fallback to not transcoding since the encoding is UTF-8 on sensible platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented a universal algorithm (for 2 and 4 bytes wchar_t and char32_t).

include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit a59678f into fmtlib:master Jun 14, 2021
@vitaut
Copy link
Contributor

vitaut commented Jun 14, 2021

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants