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