Page MenuHomePhabricator

[clang-tidy] Add --use-color command line option and UseColor option to control colors in diagnostics
ClosedPublic

Authored by hyd-dev on May 6 2020, 3:51 AM.

Details

Summary

This patch adds --use-color command line option and UseColor option to clang-tidy to control colors in diagnostics. With these options, users can force colorful output. This is useful when using clang-tidy with parallelization command line tools (like ninja and GNU parallel), as they often pipe clang-tidy's standard output and make the colors disappear.

Diff Detail

Event Timeline

hyd-dev created this revision.May 6 2020, 3:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 3:51 AM
hyd-dev edited projects, added Restricted Project; removed Restricted Project.May 6 2020, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 4:04 AM
hyd-dev removed a project: Restricted Project.May 6 2020, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 4:11 AM
hyd-dev retitled this revision from [clang-tidy] Add --color-diagnostics option to control colors in diagnostics to [clang-tidy] Add --color-diagnostics command line option and ColorDiagnostics option to control colors in diagnostics.May 6 2020, 4:33 AM
hyd-dev edited the summary of this revision. (Show Details)
hyd-dev edited the summary of this revision. (Show Details)
hokein added a comment.May 6 2020, 8:45 AM

mostly good, just nits.

clang-tools-extra/clang-tidy/ClangTidyOptions.h
130

nit: ... wil be used if the terminal connected to standard output supports colors, or even simple if missing, it will be auto detect.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
233

nit: just use-color

237

nit: set cl::init(false)

hyd-dev updated this revision to Diff 262537.EditedMay 6 2020, 8:54 PM

Address the comments above.
I also removed the incorrect DefaultOptions.ColorDiagnostics = ColorDiagnostics in ClangTidyMain.cpp. That produces incorrect UseColor: false with -dump-config.

hyd-dev updated this revision to Diff 262541.EditedMay 6 2020, 9:06 PM
hyd-dev retitled this revision from [clang-tidy] Add --color-diagnostics command line option and ColorDiagnostics option to control colors in diagnostics to [clang-tidy] Add --use-color command line option and UseColor option to control colors in diagnostics.
hyd-dev edited the summary of this revision. (Show Details)

Rename clang-tools-extra/test/clang-tidy/infrastructure/color-diagnostics.cpp to use-color.cpp.

njames93 added inline comments.May 7 2020, 2:40 PM
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
99

I'm not entirely sure this option belongs in the config file. However if it will get read in here, maybe update the help string for the command line option to say it overrides the UseColor option from the .clang-tidy configuration file.

hyd-dev updated this revision to Diff 262817.May 7 2020, 11:42 PM
hyd-dev marked an inline comment as done.

Say the --use-color command line option overrides the UseColor option in .clang-tidy file in its help text.

hyd-dev added inline comments.May 7 2020, 11:44 PM
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
99

I'm not entirely sure this option belongs in the config file.

It will be useful in the config file if the user invokes clang-tidy via another tool that does not allow the user to set clang-tidy's command line options.

However if it will get read in here, maybe update the help string for the command line option to say it overrides the UseColor option from the .clang-tidy configuration file.

I've updated it.

hyd-dev marked 3 inline comments as done and an inline comment as not done.May 7 2020, 11:45 PM
aaron.ballman added inline comments.May 8 2020, 8:23 AM
clang-tools-extra/clang-tidy/ClangTidyOptions.h
130

detect -> detected

hyd-dev updated this revision to Diff 262887.EditedMay 8 2020, 9:17 AM
hyd-dev marked an inline comment as done.

Change incorrect detect to detected in the comment of UseColor option.

Could this not use the -fcolor-diagnostics and fno-color-diagnostics command line flags that clang uses for its diagnostics to keep everything the same?

hyd-dev added a comment.EditedMay 8 2020, 9:45 PM

None of the clang-tidy command line options are prefixed with -f.
This command line option used to be --color-diagnostics, but I've followed @hokein's advice to change it to --use-color.
If you mean to use the -fcolor-diagnostics option from compile_commands.json, clang-tidy and clang are two separate tools. The users may not want them to share the same setting, because they may invoke them in different environments.

None of the clang-tidy command line options are prefixed with -f.
This command line option used to be --color-diagnostics, but I've followed @hokein's advice to change it to --use-color.
If you mean to use the -fcolor-diagnostics option from compile_commands.json, clang-tidy and clang are two separate tools. The users may not want them to share the same setting, because they may invoke them in different environments.

Fair point, will this option also control the color of the diagnostics emitted by clang or just clang tidy specific diagnostics?

Fair point, will this option also control the color of the diagnostics emitted by clang or just clang tidy specific diagnostics?

This option sets DiagOpts->ShowColors to true. As I known, it controls all diagnostics reported by the clang-tidy program (by clang::tidy::(anonymous namespace)::ErrorReporter), including clang-diagnostic-* and other clang-tidy checks.

This option sets DiagOpts->ShowColors to true. As I known, it controls all diagnostics reported by the clang-tidy program (by clang::tidy::(anonymous namespace)::ErrorReporter), including clang-diagnostic-* and other clang-tidy checks.

Would a test case be needed?

clang-tools-extra/test/clang-tidy/infrastructure/use-color.cpp
11

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

Would a test case be needed?

clang::tidy::clangTidyMain() shows that clang-tidy reports all diagnostics by clang::tidy::handleErrors(), which constructs a clang::tidy::(anonymous namespace)::ErrorReporter, no matter where they come from, so I think a test case is not required.

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

What does "option itself" mean?

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

What does "option itself" mean?

What I mean to say is, if the behaviour of the testing environment changes to pipe the result to a terminal that supports color, it could cause this test case to also fail.

What I mean to say is, if the behaviour of the testing environment changes to pipe the result to a terminal that supports color, it could cause this test case to also fail.

The standard output is always piped to FileCheck's standard input. It will never become "a terminal that supports color".
Standard error is not redirected explicitly, but it is not used to detect color support.

hyd-dev marked 2 inline comments as done.May 19 2020, 12:35 AM

Any reply?

hyd-dev edited the summary of this revision. (Show Details)May 21 2020, 8:39 AM
hyd-dev edited the summary of this revision. (Show Details)May 21 2020, 8:42 AM

Fix typos in the summary.

This revision is now accepted and ready to land.Jun 18 2020, 6:35 AM

I don't have commit access. @njames93 Could you please commit it for me (hyd-dev <yd-huang@outlook.com>)? Thanks!

This revision was automatically updated to reflect the committed changes.