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

[Zachary Turner via llvm-commits]

Ironically enough, I noticed that the implementation of `printBinaryImpl`
uses some lower-cased variables.  I'm not sure if I wrote that or someone
else, but either way, I guess don't take it as an indication that it's ok.
That's still wrong, even if it was me :)

On Tue, Nov 8, 2016 at 10:25 AM Zachary Turner <zturner at google.com> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161108/1e0c264e/attachment.html>