[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:
> rnk wrote:
> > 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?
> In that case call the variable `IsWindowsConsole` perhaps.
Done, thanks, that was something I thought about but didn't write down.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:650
+    DWORD ActuallyWritten;
+    bool Success = ::WriteConsoleW((HANDLE)::_get_osfhandle(FD), &WideText[0],
+                                   WCharsToWrite, &ActuallyWritten,
----------------
rsmith wrote:
> zturner wrote:
> > Can you test this with the following?
> > 
> > ```
> > outs() << "Test\nTest2";
> > ```
> > 
> > I'm wondering if the LF will be handled properly.  I think part of what `::write` does is to transcode LF to CRLF.  So if we're directly calling `WriteConsole`, we might not get this for free anymore.
> I think you want `&WideText[WCharWritten]` here.
Thanks. Unfortunately, this isn't testable without faking up some kind of console.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:650-652
+    bool Success = ::WriteConsoleW((HANDLE)::_get_osfhandle(FD), &WideText[0],
+                                   WCharsToWrite, &ActuallyWritten,
+                                   /*Reserved=*/nullptr);
----------------
rnk wrote:
> rsmith wrote:
> > zturner wrote:
> > > Can you test this with the following?
> > > 
> > > ```
> > > outs() << "Test\nTest2";
> > > ```
> > > 
> > > I'm wondering if the LF will be handled properly.  I think part of what `::write` does is to transcode LF to CRLF.  So if we're directly calling `WriteConsole`, we might not get this for free anymore.
> > I think you want `&WideText[WCharWritten]` here.
> Thanks. Unfortunately, this isn't testable without faking up some kind of console.
I think the Windows console doesn't care if it gets \r\n or \n at this point. I don't think there's a way I can observe a difference in behavior now, since I can't WriteConsole to a pipe. =/


https://reviews.llvm.org/D51558