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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



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


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398