[PATCH] D65191: [llvm-objdump] Implement highlighting

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 4 inline comments as done.
seiya added inline comments.


================
Comment at: llvm/include/llvm/Support/WithColor.h:100
 
+  /// Return whether --color option is set to true.
+  static bool isColorOptionSet();
----------------
Changes to WithColor.cpp and WithColor.h should be a separate patch, but before creating that I'd like to hear your comments.


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:5
+# Make sure it highlights symbol names.
+# CHECK: {{614: .* callq   -31.\[0;1;33m <foo>.\[0m}}
+
----------------
jhenderson wrote:
> I might be missing something, but how does this show that the syntax highlighting works? Is it cross-platform?
Oh I should have added `shell` to REQUIRES. This test makes sure that some elements in the disassembly are highlighted by checking ANSI escape sequences.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335
 
+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
MaskRay wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > --color is a more common option name: ls, less, grep, zstd, etc
> > > 
> > > Can you investigate a bit if it is possible to have both --color and --color=when ?
> > LLVM already has a --color option (see for example llvm-dwarfdump output, WithColor::warning() and error() calls etc etc).
> > 
> > I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).
> It looks lib/Support/WithColor.cpp#L106 already handles this...
> 
> ```
> bool WithColor::colorsEnabled() {
>   if (DisableColors)
>     return false;
>   if (UseColor == cl::BOU_UNSET)
>     return OS.has_colors();  // e.g. is a terminal
>   return UseColor == cl::BOU_TRUE;
> }
> ```
> I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).

What do you think about the compatibility with GNU objdump? Making highlighting enabled by default is incompatible behavior with GNU objcopy. Using with objdump in script (e.g., `llvm-objdump -d foo | grep bar`) won't be broken (it checks if stdout is a tty) so it won't be problem I think.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335
 
+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
seiya wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > MaskRay wrote:
> > > > --color is a more common option name: ls, less, grep, zstd, etc
> > > > 
> > > > Can you investigate a bit if it is possible to have both --color and --color=when ?
> > > LLVM already has a --color option (see for example llvm-dwarfdump output, WithColor::warning() and error() calls etc etc).
> > > 
> > > I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).
> > It looks lib/Support/WithColor.cpp#L106 already handles this...
> > 
> > ```
> > bool WithColor::colorsEnabled() {
> >   if (DisableColors)
> >     return false;
> >   if (UseColor == cl::BOU_UNSET)
> >     return OS.has_colors();  // e.g. is a terminal
> >   return UseColor == cl::BOU_TRUE;
> > }
> > ```
> > I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).
> 
> What do you think about the compatibility with GNU objdump? Making highlighting enabled by default is incompatible behavior with GNU objcopy. Using with objdump in script (e.g., `llvm-objdump -d foo | grep bar`) won't be broken (it checks if stdout is a tty) so it won't be problem I think.
> 
> Can you investigate a bit if it is possible to have both --color and --color=when ?
Did you mean whether can we allow both `--color` and `--color=true` in command-line options or not and how it is handled in colorsEnabled?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65191/new/

https://reviews.llvm.org/D65191