[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
libcxx-commits at lists.llvm.org
Mon Apr 17 09:17:44 PDT 2023
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
More information about the libcxx-commits
mailing list