D62056 makes the output color if clang auto-detects a tty, but if it
does not, there is no way to force it to use colors anyway.
This patch adjusts the command-lines given to ClangTool which will
force color on or off if --use-color is specified.
tomrittervg on Wed, Jan 13, 11:31 AM.Authored by
Thank you for working on this! I think this change should have some test coverage in clang-tools-extra/test/clang-query where a test file that emits some output is run with and without the --use-color option enabled, if possible. IIRC, there are some clang tests that you can probably use to find out the right command line arguments to pass to get the ANSI escape codes to print out.
I thought we already did? At least, for me on Windows, the effects of the patch are that I can pass --use-colors=0 to disable colors, but if I don't pass --use-colors or if I pass --use-colors=1, I get colors.
I also don't think we need to add unit tests for this, just because we can. The tests would be more complex than the code and wouldn't add much value. https://softwareengineering.stackexchange.com/a/147342 There are lots of resources about this.
Actually, I think I need to be smarter than changing the default. We want to let clang auto-detect the tty and behave that way by default if the option isn't specified. Otherwise you'd get ASNI color codes when you pipe to a file.
+1 to this, but also, you need to thread the option through to the diagnostics engine as well.
For reference, here are some screenshots of what I'm seeing on Windows with your original patch applied when running clang-query and clang-tidy. Note the difference in diagnostic text colorization where clang-tidy controls it via use-color and clang-query currently does not.
Thanks for this, I think it's looking more promising! I'd still like to see some test coverage for the changes so long as it doesn't turn into a slog. Tests like clang\test\AST\ast-dump-color.cpp show roughly how it's done in the frontend. If it turns out that the tests are too much of a pain to write for clang-query, it won't be a blocker.
I started trying to work on a patch, but I'm still unpacking from a move and don't have all my machines set up - trying to enable and build tests filled up my hard drive (even after I removed the easy-to-remove stuff), so I don't think I'm going to be able to create a test for this in the short-term.