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

Tue Oct 27 06:55:52 PDT 2020

aaron.ballman added a comment.

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

> Added a few inline comments for clarification.
>
> **Note**: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 <https://reviews.llvm.org/D79477> runs on Windows, but  in theory it should be broken on Windows since it expects ANSI escape codes in the output.

The functionality test specifies `REQUIRES: ansi-escape-sequences` so I'm guessing that test is not run on Windows. The unit test is parsing configuration options, not exercising the options themselves.

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)?

================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:113
     DiagPrinter->BeginSourceFile(LangOpts);
+#if defined(_WIN32)
+    if (DiagOpts->ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
----------------
dsanders11 wrote:
> To prevent any unintended changes this has been confined to only Windows, but the code should be safe to run on any platform. I believe it would be a no-op on Unix, which always uses ANSI escape codes and UseANSIEscapeCodes is a no-op.
I agree that the code should work on all supported platforms, I'd say you can remove the `#if`.

================
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:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90109