[libcxx-commits] [PATCH] D146398: [libcxx] Fix using std::wcout/wcin on Windows with streams configured in wide mode

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D146398#4274336 <https://reviews.llvm.org/D146398#4274336>, @Mordante wrote:

> In D146398#4267374 <https://reviews.llvm.org/D146398#4267374>, @mstorsjo wrote:
>
>> Changed to use is_same_v<char_type, wchar_t> instead of a sizeof check.
>>
>> Btw, should the added tests be under the libcxx tree, or in place under the std tree? It doesn't test any nonstandard libc++ APIs, but it uses Windows specifics to set up the test environment.
>
> Typically we still do these under std, but if the setup code uses non-standard behaviour then libcxx would be a better place.
> In general all tests under `std` should work out-of-the box with other Standard libraries.

Thanks; these tests should indeed pass with MS STL and GNU libstdc++, on Windows.



================
Comment at: libcxx/src/std_stream.h:87
+#if defined(_LIBCPP_WIN32API)
+    if constexpr (is_same_v<char_type, wchar_t>) {
+        __encoding_ = 1;
----------------
Mordante wrote:
> Since we conditionally support `wchar_t` I have a preference to check whether the type is not `char`.
Sure, I can change that.


================
Comment at: libcxx/src/std_stream.h:155
+          // know), let it know that it doesn't need to instantiate this below.
+          if constexpr (is_same_v<char_type, char>)
+#endif
----------------
Mordante wrote:
> How bad would it be to generate this dead code? It current code looks a bit tricky.
> 
> How do you feel about adding a helper variable
> ```
> #if defined(_LIBCPP_WIN32API)
> template<class CharT>
> inline constexpr bool __is_win32api_wide_char = !is_same_v<_CharT, char>;
> #else
> template<class>
> inline constexpr bool __is_win32api_wide_char = false;
> #endif
> ```
> 
> and use that? That also removes the `#ifdef`. Can you test locally whether that makes the code better readable?
Yes, this is very much not great, but unfortunately we need to avoid generating this dead code here, unless we do much larger refactorings here. Maybe there's a better way to do it though?

We need to change the type of `__extbuf` (line 125) in order to not truncate the chars.

If we try to generate dead code for this block here, the `__cv_->in` call below fails since `__extbuf` has got the wrong type for the conversion source for the `codecvt`. `__cv_` is defined as `const codecvt<char_type, char, state_type>* __cv_;` Ideally, I'd prefer to make it `const codecvt<char_type, char_type, state_type>* __cv_;` for Windows, i.e. making it a no-op (which we wouldn't call anyway), but `codecvt` doesn't support that, it only supports combinations where the second type is `char` - it can't be `<wchar_t, wchar_t>`.

So by setting `__always_noconv_` we avoid trying to use the `codecvt` at runtime, but we need to skip actual code generation for this case here...

Thanks for the tip with `__is_win32api_wide_char`, let me try that! That could simplify this (and other cases) a fair bit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146398/new/

https://reviews.llvm.org/D146398