[PATCH] D26405: Add the ability to dump hex bytes with optional offsets and ASCII bytes.

[Zachary Turner via llvm-commits]

zturner accepted this revision.
zturner added a comment.

My only other suggestion is to terminate the output when the last byte is printed rather than printing spaces.  Since `isprint(' ')` will be true, you won't be able to distinguish trailing spaces in the input from simply running out of bytes.  For example, if you tried to print the string "   " then your ascii would just look like `|                |`, and it doesn't provide any value.  So I would print that like this instead: `|   |`.  Even if the last row doesn't line up, I think that's more clear.  For example, the code I linked to earlier prints this:

  0000: 942E3101 E207E554 01000000 0B355641  |..1....T.....5VA|
  0010: 86A0A249 896F9988 FAE52FF0 22000000  |...I.o..../."...|
  0020: 2F4C696E 6B496E66 6F002F6E 616D6573  |/LinkInfo./names|
  0030: 002F7372 632F6865 61646572 626C6F63  |./src/headerbloc|
  0040: 6B000300 00000600 00000100 00001A00  |k...............|
  0050: 00000000 00001100 00000900 00000A00  |................|
  0060: 00000D00 00000000 00000500 00000000  |................|
  0070: 00004191 3201                        |..A.2.|

And that makes it clear from the ASCII that the byte sequence terminates there instead of having some trailing spaces in it.

Feel free to commit after that.



================
Comment at: include/llvm/Support/Format.h:208
+  llvm::ArrayRef<uint8_t> Bytes;
+  uint64_t FirstByteOffset; // If not -1ULL start each line with an offset.
+  uint32_t NumPerLine;      // Number of bytes to show per line.
----------------
clayborg wrote:
> zturner wrote:
> > This should probably be a `size_t`.  Also, maybe it can be an `Optional<size_t>`, which seems to more closely model what we want to use it for.
> Good idea on the optional. It probably won't use a size_t since that can be 32 bits on some systems and might prevent you from showing a 64 bit address/offset in your output.
Ahh that's a good point.


https://reviews.llvm.org/D26405