dblaikie added a comment. In D90109#2356317 <https://reviews.llvm.org/D90109#2356317>, @aaron.ballman wrote:
> 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)? +1, I'd expect such a test could unconditionally FileCheck the output to test for the escape sequences being present there. 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
