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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335
 
+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
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;
}
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:348
 
+enum class ObjdumpHighlightColor {
+  Immediate,
----------------
Maybe just `ObjdumpColor`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:372
+    std::pair<raw_ostream::Colors, bool> Pair = HighlightColors[Color];
+    outs().changeColor(Pair.first, Pair.second);
+  }
----------------
Use WithColor::changeColor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191