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

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 2 inline comments as done.
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);
----------------
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);
```


================
Comment at: llvm/lib/Support/WithColor.cpp:102
     return OS.has_colors();
   return UseColor == cl::BOU_TRUE;
 }
----------------
jhenderson wrote:
> 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.
> 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...
That makes sense. I'll rename it to `colorsEnabled`.

> Note that colorsEnabled() handles this case via DisableColors, I believe.
`DisableColors` is not related to the `--color` option: it disables colors regardless of the option:

```
  /// @param DisableColors Whether to ignore color changes regardless of -color
  /// and support in OS
  WithColor(raw_ostream &OS, HighlightColor S, bool DisableColors = false)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191