[PATCH] D51558: [Windows] Convert from UTF-8 to UTF-16 when writing to a Windows console

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added inline comments.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:572
+#ifdef _WIN32
+  IsConsole = ::GetFileType((HANDLE)::_get_osfhandle(fd)) == FILE_TYPE_CHAR;
+#endif
----------------
zturner wrote:
> You can use `llvm::sys::Process::FileDescriptorIsDisplayed(fd)` here.  Also, no need to put it behind the `#ifdef`, the member variable can be set correctly regardless of platform.
We need to differentiate between the case where the output is being displayed in mintty and a Windows console. FileDescriptorIsDisplayed() could reasonably return true for mintty, which uses pipes, and then we would try to use WriteConsoleW on a pipe. It's true that today FileDescriptorIsDisplayed will return false inside mintty, but someone might come along and fix that bug at some point.

I really want to be explicit that this is just for Windows console devices, and not some general cross-platform "is displayed" boolean, which is why the variable is IsConsole and not IsATTy. WDYT?


================
Comment at: llvm/lib/Support/raw_ostream.cpp:632
+// probably the best compromise we can make.
+static bool write_console_impl(int FD, const char *Ptr, size_t Size) {
+  SmallVector<wchar_t, 256> WideText;
----------------
zturner wrote:
> `StringRef Data`?
Sure, but it just moves it to the caller.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:755-780
+#if defined(_WIN32)
+  // Disable buffering for console devices. Console output is re-encoded from
+  // UTF-8 to UTF-16 on Windows, and buffering it would require us to split the
+  // buffer on a valid UTF-8 codepoint boundary. Terminal buffering is disabled
+  // below on most other OSs, so do the same thing on Windows and avoid that
+  // complexity.
+  if (IsConsole)
----------------
zturner wrote:
> Given that we already have this `IsConsole` variable, combined with my suggestion above to set the variable to a real value for every platform, can we re-write this entire function:
> 
> ```
> if (IsConsole)
>   return 0;
> 
> #if !defined(__minix)
>   struct stat statbuf;
>   if (fstat(FD, &statbuf) != 0)
>     return 0;
>   return statbuf.st_blksize.
> #else
>   return raw_ostream::preferred_buffer_size();
> #endif
> ```
If we do cache tty-ness, we could do that, but that's not what IsConsole is.


https://reviews.llvm.org/D51558