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

[Greg Clayton via llvm-commits]

clayborg added a comment.

Let me know if things look good Zach.



================
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.
----------------
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.


================
Comment at: include/llvm/Support/Format.h:231
+
+inline FormattedHexBytes format_hex_bytes_with_ascii(
+    llvm::ArrayRef<uint8_t> Bytes,
----------------
I think two functions is clearer. I did remove the defaulted args in the ctor. Both default args are the same for format_hex_bytes and format_hex_bytes_with_ascii.


https://reviews.llvm.org/D26405