[PATCH] D159162: [llvm] Add assembly syntax highlighting

Thu Aug 31 23:14:22 PDT 2023

MaskRay added inline comments.

================
Comment at: llvm/test/MC/Disassembler/AArch64/colored.txt:4
+
+0xa1 0x00 0x00 0x54
+# CHECK:      b.ne	[0;31m#20[0m
----------------
When testing something that is format sensitive, we want one test to test every space and typically use `--strict-whitespace --match-full-lines`, e.g. `llvm/test/tools/llvm-readobj/ELF/relocations.test`

================
Comment at: llvm/test/MC/Disassembler/AArch64/colored.txt:6
+# CHECK:      b.ne	[0;31m#20[0m
+
+0x00 0x7c 0x00 0x13
----------------
I wonder whether this is sufficient to cover changes? Things like `printImmSVE` seem uncovered?

================
Comment at: llvm/tools/llvm-mc/llvm-mc.cpp:550
     auto FOut = std::make_unique<formatted_raw_ostream>(*OS);
+    // FIXME: Workaround for buffering bug in formatted_raw_ostream.
+    if (Action == AC_CDisassemble)
----------------
Document this a bit in the description?

================
Comment at: llvm/tools/llvm-mc/llvm-mc.cpp:600
+  case AC_CDisassemble:
+    assert(IP && "Expected assembly output");
+    IP->setUseColor(true);
----------------
IP is immediately dereferenced, so the assert seems unneeded (even if `AC_MDisassemble` uses it as well)

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

https://reviews.llvm.org/D159162