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

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
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);
+}
----------------
zturner wrote:
> 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.
Which default argument value is different between the two free functions? I think they are identical.

The default is different on the class ctor though, and it may even be possible to remove the default there as the two free functions are making the uses "nice" enough.




https://reviews.llvm.org/D26405