[PATCH] D65191: [llvm-objcopy] Implement highlighting
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 24 05:48:59 PDT 2019
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
More information about the llvm-commits
mailing list