[PATCH] D56083: [llvm-objdump] - Implement -z/--disassemble-zeroes

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

A few suggested comment improvements and one function name from me. Otherwise looks good.



================
Comment at: test/MC/X86/disassemble-zeroes.s:5
+// The exact rules of skipping the bytes you can find in the code.
+// This test checks that we follow these rules and that can force
+// dissasembly of zero blocks with the -z and --disassemble-zeroes options.
----------------
and that can force -> and can force


================
Comment at: test/MC/X86/disassemble-zeroes.s:23
+
+// Check that with -z we disassemble zeroes blocks.
+// RUN: llvm-objdump -d -z %t | FileCheck %s --check-prefix=DISASM
----------------
zeroes blocks. -> blocks of zeroes.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:272-273
+cl::opt<bool> DisassembleZeroes("disassemble-zeroes",
+                                cl::desc("Do not skip blocks of zeroes when "
+                                         "disassembling the blocks of zeroes"));
+cl::alias DisassembleZeroesShort("z",
----------------
```
"Do not skip blocks of zeroes when disassembling the blocks of zeroes"
```
I think this should be simply "Do not skip blocks of zeroes when disassembling"


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1309-1311
+// Normally the disassembly output will skip blocks of zeroes. This function for
+// the given buffer Buf with the instructions returns the number of zero bytes
+// that can be skipped during dumping the disassembly output.
----------------
I'd reword the second sentence as:

"This function returns the number of zero bytes that can be skipped when dumping the disassembly of the instructions in Buf."


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1312
+// that can be skipped during dumping the disassembly output.
+static size_t skipZeroesBlock(ArrayRef<uint8_t> Buf) {
+  // When -z or --disassemble-zeroes are given we always dissasemble them.
----------------
Since this function isn't actually doing the skipping, perhaps this should be something like "countSkippableZeroBytes" or something along those lines?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1317
+
+  // Find the amount of leading zeroes.
+  size_t Amount = 0;
----------------
amount -> number

Also, please rename the variable accordingly.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1322-1323
+
+  // We may want to skip blocks of zero bytes, but not earlier
+  // than we see at least 8 of them in a row.
+  if (Amount < 8)
----------------
earlier than -> unless (I think)


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

https://reviews.llvm.org/D56083