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

[Zachary Turner via llvm-commits]

zturner added a comment.

Mostly looks good, just a few nits and I think we're good.



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


================
Comment at: include/llvm/Support/Format.h:230-235
+inline FormattedHexBytes format_hex_bytes_with_ascii(
+    llvm::ArrayRef<uint8_t> Bytes, uint64_t FirstByteOffset = -1ULL,
+    uint32_t NumPerLine = 16, uint8_t ByteGroupSize = 4) {
+  return FormattedHexBytes(Bytes, FirstByteOffset, NumPerLine, ByteGroupSize,
+                           false, true);
+}
----------------
It's a bit confusing to me that there are two almost identical versions of the function with different default values for the parameters.  What about having a single version that takes all parameters?  If so, I would suggest the signature be something like:

`format_hex_bytes(ArrayRef<uint8_t>, bool ShowAsciiBlock = true, uint32_t BytesPerLine=16, uint8_t BytesPerGroup = 4, bool Upper = true, Optional<size_t> FirstByteOffset = 0)`.

I'm not sure if this is the optimal ordering of parameters, but I tried to arrange this in order from most commonly specified to least commonly specified.  Not sure if I got it right though.

If you think it's better to have two versions of the functions, at least make the default values of the arguments be the same.


https://reviews.llvm.org/D26405