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

Wed Oct 28 15:15:42 PDT 2020

dsanders11 added a comment.
Herald added a subscriber: dexonsmith.

> 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.

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?

================
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;
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90109