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

[Zachary Turner via llvm-commits]

zturner added a comment.

Just a tip.  If you wrap the block in triple backticks it will show up in a fixed-width font in phabricator (see below for an example)

That aside, this looks pretty good, but we have this functionality in a couple of places in LLVM already.  It would be nice to centralize.  Also, this is just a personal preference, but I kind of prefer the way the existing implementations format this.  Maybe you could look at the existing implementations and if you prefer it as well, move the code for them over here?

Here's a sample of what our current output looks like:

  bin\llvm-pdbdump raw -stream-data=1 d:\src\llvm\test\debuginfo\pdb\inputs\empty.pdb
  ...
        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.|

The code for this is in `ScopedPrinter::printBinaryImpl`.

Even if we don't combine the two right away, I still prefer this style, so it would be nice if the API matched this style closely enough that it would be possible to have `printBinaryBlock` delegate to your function in the future with only minor changes.

Some limitations of the existing implementation:

1. Doesn't allow configurable line length.  It's always 16-bytes per line.
2. Doesn't allow configurable field-width.  Always 4-bytes per field.
3. Doesn't allow prefixing the address with `0x` or left-padding with zeros.

Feel free to take on as much or as little of this as you think is useful.  But I think we should at least be consistent with the output style.



================
Comment at: lib/Support/raw_ostream.cpp:370
+    // Print the hex bytes for this line
+    uint32_t i = 0;
+    for (i = 0; i < FB.NumPerLine; ++i) {
----------------
This will need to be an uppercase `I`.


================
Comment at: lib/Support/raw_ostream.cpp:388
+      // Print the ASCII char values for each byte on this line
+      for (uint32_t i = 0; i < FB.NumPerLine; ++i) {
+        size_t Index = LineIndex + i;
----------------
Same here.


================
Comment at: unittests/Support/NativeFormatTests.cpp:48
 
+std::string format_hex_bytes(const void *P, size_t N, uint64_t Offset = -1ULL,
+                             uint32_t NumPerLine = 16) {
----------------
I know it's a poorly named file, but these tests should probably go in `raw_ostream_test.cpp`.  `NativeFormatTests.cpp` was intended specifically to test the functions in `NativeFormatting.cpp`.


Repository:
  rL LLVM

https://reviews.llvm.org/D26405