[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/lib/Support/WithColor.cpp:102
     return OS.has_colors();
   return UseColor == cl::BOU_TRUE;
 }
----------------
jhenderson wrote:
> seiya wrote:
> > 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)
> > ```
> Under what situations can `OS.has_colors()` return false? I'm not convinced we want to return true if a stream doesn't have colours, even if the switch has been specified.
In UNIX,`OS.has_colors()` returns false if either the stdout is not a tty (say, pipe) or the it does not support colors. Even if it does not support colors, it's sometimes useful to enable colors with the switch. 

For example, in `disassembly-highlighting.test`,  since the stdout of llvm-objdump is a pipe to FileCheck, `OS.has_colors()` will return false. However, in order to check that the output includes colors, we need a way to force colors by the switch.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:368
+
+/// a context for WithNestedColor which holds saved colors.
+class WithNestedColorContext {
----------------
jhenderson wrote:
> a -> A
> 
> Would it make sense for this code to exist in WithColor.h/.cpp?
> 
> In fact, I wonder if this should be used by WithColor directly, i.e. have WithColor restore the previous state by default, rather than the plain version.
> Would it make sense for this code to exist in WithColor.h/.cpp?

I think so too. I'll submit this part as a separate patch.

> In fact, I wonder if this should be used by WithColor directly, i.e. have WithColor restore the previous state by default, rather than the plain version.
That sounds good to me. I'll try it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191