The colour characters currently added to the output of -print-changed=diff and -print-changed=diff-quiet cause difficulties when capturing the output and examining it in an editor. Change the function to not have the colour characters and add 2 new choices (-print-changed=cdiff and -print-changed=cdiff-quiet) to retain the existing functionality of adding the colour characters.
Details
Diff Detail
Unit Tests
Event Timeline
There's already color autodetection in clang somewhere, would that be useful as a default?
llvm/test/Other/ChangePrinters/print-changed-diff.ll | ||
---|---|---|
65 | I would have expected to see some diffs in the existing CHECK lines since they were previously emitting color directives, but now aren't Am I missing something? | |
106 | -disable-output, although I guess it's a bit late to change all occurrences, at least in this change |
I don't think we want auto-detection because the user needs to be able to control it. If it is just going to the screen, the colour is useful, but if the output is being captured and brought up in emacs/vim/etc, the colour characters just show as control characters. less on LINUX doesn't handle the colour characters; other utilities may also have problems with them.
llvm/test/Other/ChangePrinters/print-changed-diff.ll | ||
---|---|---|
65 | The existing tests didn't need to be changed because they only looked for +/- and didn't check for the colour characters. In adding the new tests, I copied the existing ones and added in a regex that checks for (most of) the colour control characters. |
I think the autodetection detects if the output supports color or not. e.g. https://clang.llvm.org/docs/UsersManual.html:
-f[no-]color-diagnostics This option, which defaults to on when a color-capable terminal is detected, controls whether or not Clang prints diagnostics in color.
Sure, the auto-detection could be used as you suggest, but the control is still needed by the user. Consider what happens when auto-detection says the system supports colour and the user captures the output to a file. They then bring it up in an editor (say vim or emacs) which doesn't recognize the colour characters and show as ^[[31m- and ^[[31m+ (in emacs or vim). By giving the user control over whether they are generated, they can include them if they are examining the results on a screen (or using more) or strip them out when they plan to capture and edit the results (for large changes).
I think defaulting to autodetect with an option to explicitly choose either is ideal, even though that introduces more options; but doesn't have to be done here, this is an improvement on its own.
I would have expected to see some diffs in the existing CHECK lines since they were previously emitting color directives, but now aren't
Am I missing something?