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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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}}
----------------
jhenderson wrote:
> 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.
We usually start comment from `##` nowadays.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:353
+  // Reset colors.
+  Reset,
+};
----------------
It is probably not clear what is `Reset`. Can it be `Default`?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:357
+// TODO: Make this configurable (just like $LS_COLORS).
+std::map<ObjdumpHighlightColor, std::pair<raw_ostream::Colors, bool>> HighlightColors = {
+  // Default colors: Color, { raw_ostream::Colors, Bold }
----------------
Make it const?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:371
+  if (HighlightColors.count(Color)) {
+    std::pair<raw_ostream::Colors, bool> Pair = HighlightColors[Color];
+    outs().changeColor(Pair.first, Pair.second);
----------------
It is generally not a good practice to do a lookup twice.
You can use `std::map::find()` to avoid that.

(Also I do not think you will be able to use `operator[]` after making `HighlightColors` `const`)


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1126
+  for (const MarkupSpan &Span : Spans) {
+    StringRef BeforeText = Text.substr(NextPos, Span.Pos - NextPos);
+    outs() << BeforeText;
----------------
You use `BeforeText` only once, so you do not need it it seems (just inline it).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1140
+        break;
+      }
+
----------------
Seems this would better look as `if`s:

```
ObjdumpHighlightColor Color;
if (Span.Type == MarkupType::Imm)
  Color = ObjdumpHighlightColor::Immediate;
else if (Span.Type == MarkupType::Reg)
  Color = ObjdumpHighlightColor::Register;
else
  Color = ObjdumpHighlightColor::Reset;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65191