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

[Seiya Nuta via Phabricator via llvm-commits]

seiya 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);
----------------
seiya wrote:
> jhenderson wrote:
> > I think this needs rephrasing. How about "Determine whether the specified stream supports colors."?
> That sounds not sufficient to me. `colorsAvailable` returns true if the specified stream supports colors //or// `--color=1` is given. As you suggested in another comment, I think adding another `colorsEnabled()` would be better:
> 
> ```
>   /// Determine whether colors are displayed.
>   bool colorsEnabled();
>   static bool colorsEnabled(raw_ostream &OS);
> ```
Updated to "Determine whether colors are displayed..." instead of "... stream supports colors" because even if the stream supports colors, it can be disabled by `-color=false`.


================
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.
+
----------------
seiya wrote:
> jhenderson wrote:
> > 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.
> Good catch! I'll fix this.
I just remembered that I added `--color` intentionally because stdout is not a tty. I've updated the comment:  "... by default" → "... if colors are enabled".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191