PATCH] [clang-query] Add a --use-color option to clang-query to allow forcing the behavior

[tomrittervg]

Summary

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.

[aaron.ballman]

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.

[steveire]

Can we make using color the default too? We already use colors for Decls I think, so this just adds colors for other Node types.

[aaron.ballman]

In D94624#2497992, @steveire wrote:

Can we make using color the default too? We already use colors for Decls I think, so this just adds colors for other Node types.

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.

[steveire]

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.

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Doesn't this set the default to false?

[aaron.ballman]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

It does, but it does not disable colors by default (I had to double-check this myself), which is why I was asking for the regression tests. That will clearly show the behavior and help ensure we don't regress it in the future.

[steveire]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Did you build and run with the patch?

Current clang-query dumps with color. After this patch, when not using the command line option, there is now no color. with -use-color, the color is restored.

@tomrittervg I know this is based on our discussions, but it's not clear to me what the intent of the patch is. Do you want to be able to disable color?

[tomrittervg]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Let me double check - my current clang-query did not dump with color (which is why I made the patch). Maybe this was a mix-up I had.

[steveire]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

https://reviews.llvm.org/D62056 already made dumps use color.

[tomrittervg]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Is that what you're running in your CE instance? You're right, I forgot about that; and that does make it output color on the terminal. But not CE.

I _think_ what's happening is that CE isn't acting like a tty, so clang won't output it in color even if I specify --extra-arg="-fdiagnostics-color" (which the analogous --extra-arg="-fno-diagnostics-color" _will_ turn off color on the command-line.)

[steveire]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Yes, you're probably right that it's tty related.

It looks to me like the only problem is that you specified the default as false instead of true.

[aaron.ballman]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

58

Did you build and run with the patch?

Yes.

Current clang-query dumps with color. After this patch, when not using the command line option, there is now no color. with -use-color, the color is restored.

I think we're looking at different parts of the output. Diagnostic text and underlines are always colored for me, regardless of what options I pass. AST dump output only gets controlled by this new flag, which is different from how it works in clang-tidy, where --use-color=0 will disable colors from diagnostic text output as well.

So I think there are two issues: 1) This isn't controlling all of the colors, 2) The default on/off value is no longer tied to whether the output is able to display colors.

[tomrittervg]

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.

[aaron.ballman]

In D94624#2498201, @tomrittervg wrote:

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.

[tomrittervg]

In D94624#2498244, @aaron.ballman wrote:

In D94624#2498201, @tomrittervg wrote:

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.

Let me fiddle with this, I think I have an idea of how to do that.

[tomrittervg]

Okay, completely new direction that handles things better I think.

[aaron.ballman]

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.

clang-tools-extra/clang-query/Query.cpp

159

Semi-idle curiosity, does the source manager have a different diagnostics object than the AST? I guess I'm a bit surprised this change is needed (or is the change just a minor simplification to the code?).

clang-tools-extra/clang-query/tool/ClangQuery.cpp

124–128

Our coding style typically has us elide braces around single-line constructs like this.

[tomrittervg]

Remove braces

[tomrittervg]

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.

clang-tools-extra/clang-query/Query.cpp

159

Just a minor simplification.

[aaron.ballman]

In D94624#2501894, @tomrittervg wrote:

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.

Okay, I think this LGTM even without the test coverage. Thank you for the patch, do you need someone to commit it on your behalf?

[tomrittervg]

In D94624#2506831, @aaron.ballman wrote:

In D94624#2501894, @tomrittervg wrote:

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.

Okay, I think this LGTM even without the test coverage. Thank you for the patch, do you need someone to commit it on your behalf?

Yes, please.

[aaron.ballman]

In D94624#2506906, @tomrittervg wrote:

In D94624#2506831, @aaron.ballman wrote:

In D94624#2501894, @tomrittervg wrote:

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.

Okay, I think this LGTM even without the test coverage. Thank you for the patch, do you need someone to commit it on your behalf?

Yes, please.

I'm not certain why, but the patch is not applying to a fresh pull for me.

F:\llvm-project>git apply "F:\Aaron Ballman\Desktop\D94624.diff"
error: patch failed: clang-tools-extra/clang-query/tool/ClangQuery.cpp:109
error: clang-tools-extra/clang-query/tool/ClangQuery.cpp: patch does not apply

As usual, git's not being particularly helpful in stating what went wrong. Perhaps rebasing your patch on ToT and re-uploading it will cause the issue to go away?

[tomrittervg]

Try rebasing it...

[aaron.ballman]

In D94624#2516540, @tomrittervg wrote:

Try rebasing it...

Thanks, that was sufficient for me to apply it. I've commit on your behalf in d6d36baa33e76ace11ac20c03de1097d48bd9246, thank you for the patch!

clang-tools-extra/clang-query/Query.cpp

         if (QS.DetailedASTOutput) {
           OS << "Binding for \"" << BI->first << "\":\n";
           const ASTContext &Ctx = AST->getASTContext();
-          const SourceManager &SM = Ctx.getSourceManager();
-          ASTDumper Dumper(OS, Ctx, SM.getDiagnostics().getShowColors());
+          ASTDumper Dumper(OS, Ctx, AST->getDiagnostics().getShowColors());
           Dumper.SetTraversalKind(QS.TK);
           Dumper.Visit(BI->second);
           OS << "\n";

clang-tools-extra/clang-query/tool/ClangQuery.cpp

 static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
 static cl::OptionCategory ClangQueryCategory("clang-query options");
 
+static cl::opt<bool>
+    UseColor("use-color",
+             cl::desc(
+                 R"(Use colors in detailed AST output. If not set, colors
+will be used if the terminal connected to
+standard output supports colors.)"),
+             cl::init(false), cl::cat(ClangQueryCategory));
+
 static cl::list<std::string> Commands("c", cl::desc("Specify command to run"),
                                       cl::value_desc("command"),
                                       cl::cat(ClangQueryCategory));

 
   ClangTool Tool(OptionsParser->getCompilations(),
                  OptionsParser->getSourcePathList());
+
+  if (UseColor.getNumOccurrences() > 0) {
+    ArgumentsAdjuster colorAdjustor = [](const CommandLineArguments &Args, StringRef /*unused*/) {
+      CommandLineArguments AdjustedArgs = Args;
+      if (UseColor)
+        AdjustedArgs.push_back("-fdiagnostics-color");
+      else
+        AdjustedArgs.push_back("-fno-diagnostics-color");
+      return AdjustedArgs;
+    };
+    Tool.appendArgumentsAdjuster(colorAdjustor);
+  }
+
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
   int ASTStatus = 0;
   switch (Tool.buildASTs(ASTs)) {