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

[Zachary Turner via llvm-commits]

Ahh, you are right.  It's between the free functions and the class.  But I
think you're right, we should be able to remove them from the class
entirely and just use the values on the free functions (or function, if it
seems reasonable to remove one)

On Tue, Nov 8, 2016 at 1:32 PM Mehdi AMINI <mehdi.amini at apple.com> wrote:

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