Page MenuHomePhabricator

add print-changed mode that does not use colour
ClosedPublic

Authored by jamieschmeiser on Feb 24 2021, 9:37 AM.

Details

Summary

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.

Diff Detail

Event Timeline

jamieschmeiser created this revision.Feb 24 2021, 9:37 AM
jamieschmeiser requested review of this revision.Feb 24 2021, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 9:37 AM

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 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.

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).

aeubanks accepted this revision.Feb 26 2021, 3:44 PM

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.

This revision is now accepted and ready to land.Feb 26 2021, 3:44 PM
This revision was landed with ongoing or failed builds.Thu, Mar 25, 7:36 AM
This revision was automatically updated to reflect the committed changes.