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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/WithColor.cpp:102
     return OS.has_colors();
   return UseColor == cl::BOU_TRUE;
 }
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:368
+
+/// a context for WithNestedColor which holds saved colors.
+class WithNestedColorContext {
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:370
+class WithNestedColorContext {
+  std::stack<std::pair<raw_ostream::Colors, bool>> NestedColors;
+
----------------
Consider replacing the pair with a simple struct e.g. `ColorState`. That'll make the code using it much clearer.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:397
+///     outs() << "A blue-colored text.";
+///     // WNC2 is destructed and WNC1's color is restored here.
+///   }
----------------
destructed -> destroyed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191