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