This is an archive of the discontinued LLVM Phabricator instance.

Add option to control whether llvm-pdbdump outputs in color
ClosedPublic

Authored by amccarth on Mar 22 2017, 2:51 PM.

Details

Summary

Adds -color-output option to llvm-pdbdump pretty commands that lets you specify whether the output should have color. The default depends on whether the output is going to a TTY (per prior discussion in https://reviews.llvm.org/D31246).

This will enable tests that pipe llvm-pdbdump output to FileCheck to work across platforms without regard to the differences in ANSI codes.

Diff Detail

Event Timeline

amccarth created this revision.Mar 22 2017, 2:51 PM
zturner accepted this revision.Mar 22 2017, 2:58 PM
zturner added inline comments.
llvm/tools/llvm-pdbdump/LinePrinter.h
27

Does this need a default argument if we're always going to specify it anyway?

llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp
130–131

We should probably come up with some wording that makes it clear that the user does not need to specify this to get color output if they're just running from the terminal. A way to do this concisely escapes me at the moment, however.

This revision is now accepted and ready to land.Mar 22 2017, 2:58 PM
amccarth marked an inline comment as done.Mar 22 2017, 3:13 PM
amccarth added inline comments.
llvm/tools/llvm-pdbdump/LinePrinter.h
27

I moved the UseColor parameter to the middle, in order to keep the "options" together and avoid burying the stream in the middle.

llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp
130–131

Acknowledged, but I haven't come up with a concise phrasing either. Happy to change in the future if a better idea comes along.

I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"

amccarth marked an inline comment as done.Mar 22 2017, 3:42 PM

I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"

OK, I came up with "Override use of color (default = isatty)", which I assume should be clear enough for typical users of this tool.

I'll let it bake tonight and submit in the morning, in case Chandler or anyone else has additional ideas.

chandlerc edited edge metadata.Mar 23 2017, 3:12 AM

I think we need something before you commit, because as it is people will be specifying --color-output unnecessarily. Even if you say "default = on" or "default = best-effort"

OK, I came up with "Override use of color (default = isatty)", which I assume should be clear enough for typical users of this tool.

I'll let it bake tonight and submit in the morning, in case Chandler or anyone else has additional ideas.

I have no better suggestion. Thanks for reworking with this approach though! =D

This revision was automatically updated to reflect the committed changes.