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

Sat Apr 8 09:13:50 PDT 2023

mstorsjo added a comment.

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

> Is it possible to add tests for these changes?

I’d love to, but it’s not very straightforward.. We already lack coverage of the conversion aspects of `std::cout`/`std::wcout` - the most complex thing we test is to print `1234` to the output streams and check that it ends up printing the right thing with a bash script. To test the relevant aspects here, we’d need to print unicode charts, and I’m not sure how well that integrates with bash here (or how well llvm-lit manages to pass the unicode string to validate to the script).

================
Comment at: libcxx/include/__std_stream:87
+#if defined(_LIBCPP_WIN32API)
+    if constexpr (sizeof(char_type) == sizeof(wchar_t)) {
+        __encoding_ = 1;
----------------
Mordante wrote:
> `if constexpr` fails before C++17, AFAIK this code should be working in all language modi.
Yeah, we can’t use it in public headers, but as far as I could see, this header isn’t ever accessed by any public header - it should only be included by the libcxx sources.

I think it might be fine to leave out the constexpr if you want it out of principle though, I don’t think there was a functional need for it.

================
Comment at: libcxx/include/__std_stream:124
     }
+#if defined(_LIBCPP_WIN32API)
+    char_type __extbuf[__limit];
----------------
Mordante wrote:
> Is this `ifdef` really needed? Will it fail on Linux when using `char_type`?
Yes; the type of `__extbuf` needs to be the right one for the conversion from the external charset (utf8 in a char array on other systems, but read directly as wchars on windows) into `char_type` - see the `__cv_->in` call below on line 138/164.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398