[PATCH] [libcxx] std::codecvt - short wchar support

[Oleg Ranevskyy]

In http://reviews.llvm.org/D8880#153497, @mclow.lists wrote:

> I don't like the gutting of the tests.
>  I don't like the confusion added to the code.


Codecvt_utf8 already handles short wchar properly and does this in exactly same manner. The patch adds short wchar support to other codecvt's, which seem to have been forgotten. The change is similar to how it is done in gcc's libstdc++.

The next patch changes this a bit by introducing the SHORT_WCHAR macro. This saves some typing and, I hope, makes the code more readable. Would you take a look and share your thoughts, please?

Yes, I agree about the tests. The next patch will avoid messing in the tests by splitting such wchar-size-dependent tests into two versions: for short wchar and for "long" wchar. Lit will choose which test to run depending on short_wchar feature availability, which will also be added.


REPOSITORY
  rL LLVM

================
Comment at: src/locale.cpp:1877
@@ -1946,2 +1876,3 @@
+              _ToType* to, _ToType* to_end, _ToType*& to_nxt,
               unsigned long Maxcode = 0x10FFFF, codecvt_mode mode = codecvt_mode(0))
 {
----------------
mclow.lists wrote:
> mclow.lists wrote:
> > You're changing a function named `utf8_to_utf16` to take parameters of type `uint32_t *`?
> > 
> > Eww.  I object.
> I apologize for this comment. We already have a function that does this.
> Still, eww.
> 
Objecting to the existing functionality. We already have two utf8_to_utf16 functions and two utf16_to_utf8 to support char16_t and char32_t.

This part of the patch is not a functional change. It just tries to improve code reuse instead.
As this is just an improvement, I can revert it if code repetition looks better to someone or the corrected version seems confusing.

================
Comment at: test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp:29
@@ +28,3 @@
+#if _WIN32
+    #define SIZEOF_WCHAR_T 2
+#elif __SIZEOF_WCHAR_T__ 
----------------
jroelofs wrote:
> Does win32 not have `__SIZEOF_WCHAR_T__`?
It does not seem to have `__SIZEOF_WCHAR_T__`, unfortunately.

================
Comment at: test/std/localization/locale.stdcvt/codecvt_utf16_in.pass.cpp:33
@@ +32,3 @@
+#else
+    #define SIZEOF_WCHAR_T 4
+#endif
----------------
jroelofs wrote:
> The else should really be something to the tune of: `#error Can't tell how big wchar_t is on this platform". Otherwise this is a potential nightmare for people doing ports.
Thank you for your remark. I will introduce such a message in the next patch version.

================
Comment at: test/std/localization/locales/locale.convenience/conversions/conversions.string/to_bytes.pass.cpp:28
@@ +27,3 @@
+#else
+    #define SIZEOF_WCHAR_T 4
+#endif
----------------
jroelofs wrote:
> same here. Maybe this macro should go in one of the test support headers instead?
Ok, I am going to change the way the tests are handled for short wchar, so that they won't contain these `#if / #else` anymore.

http://reviews.llvm.org/D8880

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/