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

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

Cool feature!
I only have minor comments inline.



================
Comment at: include/llvm/Support/Format.h:208
+  llvm::ArrayRef<uint8_t> Bytes;
+  uint64_t Offset; // If not -1ULL start each line with an appropriate offset.
+  uint32_t NumPerLine; // Number of bytes to show per line.
----------------
This wasn't clear to me until I read the code in raw_ostream that this is the "starting offset" for the offset to print in the margin. I thought it was about indention.


================
Comment at: include/llvm/Support/Format.h:223
+
+inline FormattedHexBytes format_hex_bytes(const void *P, size_t N,
+                                          uint64_t Offset = -1ULL,
----------------
I'm not against the "type erasure" with `void *`, but what about also keeping an overload with an ArrayRef of uint8_t? It could avoid to have to unpack at call sites.


================
Comment at: lib/Support/raw_ostream.cpp:373
+      size_t Index = LineIndex + i;
+      if (Index < Size) {
+        if (i)
----------------
Early exit:

```
if (Index >= Size)
  break;
```


================
Comment at: lib/Support/raw_ostream.cpp:390
+        size_t Index = LineIndex + i;
+        if (Index < Size) {
+          char ch = (char)FB.Bytes[Index];
----------------
This loop could iterate from 0 to i and you wouldn't need this test here.
Repeating the same early exit as the previous loop might be easier to read though.


Repository:
  rL LLVM

https://reviews.llvm.org/D26405