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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/WithColor.h:97
 
+  /// Determine whether colors are available for the given stream.
+  static bool colorsAvailable(raw_ostream &OS);
----------------
I think this needs rephrasing. How about "Determine whether the specified stream supports colors."?


================
Comment at: llvm/lib/Support/WithColor.cpp:99
 
-bool WithColor::colorsEnabled() {
-  if (DisableColors)
-    return false;
+bool WithColor::colorsAvailable(raw_ostream &OS) {
   if (UseColor == cl::BOU_UNSET)
----------------
How about naming this `colorsEnabled` too?


================
Comment at: llvm/lib/Support/WithColor.cpp:102
     return OS.has_colors();
   return UseColor == cl::BOU_TRUE;
 }
----------------
I'm not sure this clause makes sense to me? If the stream doesn't have colours, saying colours are available for it because somebody has specified `--color` seems wrong...

Note that `colorsEnabled()` handles this case via DisableColors, I believe.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassembly-highlighting.test:2
+## Show that when disassembling, llvm-objdump highlights symbol names,
+## registers, and immediate values by default.
+
----------------
The comment says "by default", but the test doesn't show the default behaviour! It only shows behaviour when `--color` or `--color=false` is specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191