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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
