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

Zachary Turner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 31 15:00:46 PDT 2018


zturner added inline comments.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:572
+#ifdef _WIN32
+  IsConsole = ::GetFileType((HANDLE)::_get_osfhandle(fd)) == FILE_TYPE_CHAR;
+#endif
----------------
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.


================
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;
----------------
rnk wrote:
> zturner wrote:
> > `StringRef Data`?
> Sure, but it just moves it to the caller.
Well sure, but my next question would be "why doesn't `write_impl` take a `StringRef`?"  Seems like something that should keep pushing up the call stack as high as possible  (Not that I'm asking you to make that change, but it just seems like the right api, despite the fact that the caller has what is, IMO, a buggy API).


================
Comment at: llvm/lib/Support/raw_ostream.cpp:650-652
+    bool Success = ::WriteConsoleW((HANDLE)::_get_osfhandle(FD), &WideText[0],
+                                   WCharsToWrite, &ActuallyWritten,
+                                   /*Reserved=*/nullptr);
----------------
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.


https://reviews.llvm.org/D51558





More information about the llvm-commits mailing list