Page MenuHomePhabricator

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

Authored by tomrittervg on Jan 13 2021, 11:31 AM.

Details

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.

Diff Detail

Event Timeline

tomrittervg requested review of this revision.Jan 13 2021, 11:31 AM
tomrittervg created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 11:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added a subscriber: 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.

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.

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.

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 added inline comments.Jan 14 2021, 5:48 AM
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 added inline comments.Jan 14 2021, 6:22 AM
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 added inline comments.Jan 14 2021, 6:24 AM
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 added inline comments.Jan 14 2021, 6:32 AM
clang-tools-extra/clang-query/tool/ClangQuery.cpp
58

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

tomrittervg added inline comments.Jan 14 2021, 6:39 AM
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 added inline comments.Jan 14 2021, 6:54 AM
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 added inline comments.Jan 14 2021, 6:57 AM
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.

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.

tomrittervg retitled this revision from [PATCH] [clang-query] Add a --use-color option to clang-query to [PATCH] [clang-query] Add a --use-color option to clang-query to allow forcing the behavior.Jan 14 2021, 7:07 AM
tomrittervg edited the summary of this revision. (Show Details)

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.

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 retitled this revision from [PATCH] [clang-query] Add a --use-color option to clang-query to allow forcing the behavior to PATCH] [clang-query] Add a --use-color option to clang-query to allow forcing the behavior.
tomrittervg edited the summary of this revision. (Show Details)

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

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.

Remove braces

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 accepted this revision.Jan 19 2021, 7:20 AM

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?

This revision is now accepted and ready to land.Jan 19 2021, 7:20 AM

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

Try rebasing it...

aaron.ballman closed this revision.Jan 25 2021, 5:08 AM

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!