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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: grimar.
jhenderson added a comment.

If you haven't already, take a look at WithColor.h. I believe it allows you to stream things in a specific colour properly, including with disabling the feature via --color. From the WithColor class description, you can use it like `WithColor(outs(), raw_ostream::CYAN) << "some text";` to print something to the stream in cyan.



================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:1
+# RUN: llvm-objdump -d --highlight %p/Inputs/highlight.elf-x86_64 | FileCheck %s
+# REQUIRES: x86-registered-target
----------------
Try and avoid adding a binary if you can. There's no particular reason this can't use yaml2obj to do things. Just add a .text section with appropriate content.


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:2
+# RUN: llvm-objdump -d --highlight %p/Inputs/highlight.elf-x86_64 | FileCheck %s
+# REQUIRES: x86-registered-target
+
----------------
REQUIRES usually go first in the output.


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:4
+
+# Make sure it highlights symbol names.
+# CHECK: {{614: .* callq   -31.\[0;1;33m <foo>.\[0m}}
----------------
Move this comment to near the top of the file, and make it a bit more verbose, so that it describes the test as a whole. Take a look at recent new tests in various of the LLVM tools for examples.


================
Comment at: llvm/test/tools/llvm-objdump/highlight.test:5
+# Make sure it highlights symbol names.
+# CHECK: {{614: .* callq   -31.\[0;1;33m <foo>.\[0m}}
+
----------------
I might be missing something, but how does this show that the syntax highlighting works? Is it cross-platform?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:335
 
+static cl::opt<bool> Highlight("highlight",
+                               cl::desc("Enable syntax highlighting"),
----------------
MaskRay wrote:
> --color is a more common option name: ls, less, grep, zstd, etc
> 
> Can you investigate a bit if it is possible to have both --color and --color=when ?
LLVM already has a --color option (see for example llvm-dwarfdump output, WithColor::warning() and error() calls etc etc).

I'm inclined to think that syntax highlighting can be on by default and just use --color=false (or whatever it is) to turn it off. If we go that route, the --color option should be documented in the llvm-objdump command guide, and also we should check it is in the tool help text (since it's in a different option category).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:698
                          StringRef Annot, MCSubtargetInfo const &STI,
-                         SourcePrinter *SP,
+                         SourcePrinter *SP, size_t &HeaderLength,
                          std::vector<RelocationRef> *Rels = nullptr) {
----------------
How about instead of "HeaderLength" this is called "InstructionCol". The thing you're measuring isn't really a "Header".


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1123
+static void printMarkupSpans(StringRef Text,
+                             const std::vector<MarkupSpan> &Spans,
+                             size_t &NextPos) {
----------------
Use ArrayRef here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191