[PATCH] D90109: [clang-tidy] Use ANSI escape codes for --use-color on Windows

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D90109#2360538 <https://reviews.llvm.org/D90109#2360538>, @dsanders11 wrote:

>> What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?
>
> I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7 <https://developercommunity.visualstudio.com/content/problem/1144371/failed-to-compile-llvm-9011001-in-vs-2019-1670.html>. There's Google results on the issue, but I don't see any solution and I'm using `master` and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

Oh, how neat. The bug you linked to was closed as a dup of another bug. The other bug was then closed as something that's not in scope with the general product direction, whatever that means.

> I also wasn't seeing any of the test targets being made by CMake until I included `-DLLVM_INCLUDE_TESTS=On`, I think maybe I set that to off when trying to determine why the debug build was failing and didn't realize that would stay in `CMakeCache.txt` until I set it back on.
>
> Removing `REQUIRES: ansi-escape-sequences` from the `clang-tidy/infrastructure/use-color.cpp` test does run it on Windows and does pass as expected with this change. Is that all you're looking for with test coverage?

Yeah, that would be reasonable test coverage.



================
Comment at: llvm/lib/Support/Windows/Process.inc:330
     DWORD Mode;
-    GetConsoleMode(Console, &Mode);
-    Mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
-    SetConsoleMode(Console, Mode);
+    if (GetConsoleMode(Console, &Mode) != 0) {
+      Mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
----------------
dsanders11 wrote:
> aaron.ballman wrote:
> > dsanders11 wrote:
> > > Fixes a crash when calling UseANSIEscapeCodes if standard out isn't a console.
> > If this fails, do we still want to set `UseANSI` to `enable` below? Similar if setting the mode fails?
> If we don't set `UseANSI` below then this patch won't do what it's intended to do, since piped stdoutwouldn't have ANSI escape codes. This code block is assuming that standard output is a console, so `GetConsoleMode` won't work on redirected stdout.
> 
> You could use `llvm::sys::Process::StandardOutIsDisplayed()` to check, but for Windows that's actually just doing the same thing, doing a `GetConsoleMode` call and seeing if it succeeds or not.
> 
> For this patch to do what it's intended to do, `UseANSIEscapeCodes` needs to turn on ANSI escape codes even if there isn't a console.
Ah, thank you for the explanation -- my complaint is more that `UseANSIEscapeCodes` can fail but doesn't alert the caller to the failure. However, that's not a new issue, so I think it's fine as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90109