[9] RFR 8170769: Provide a simple hexdump facility for binary data

[Vincent Ryan]

Thanks for your review comments Paul.
Responses below.


> On 8 Dec 2016, at 20:08, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 
> Hi,
> 
> It may take a few iterations to get the API settled.
> 
> There are other byte sources we may want to consider such as InputStream and ByteBuffer.

I’d prefer to keep this class simple for now but I’m happy to add additional methods later if there is demand.

Adding support for InputStream is awkward because of its blocking nature.
It’s simpler to allow the user to handle that aspect, populate a byte array, and present it to the HexDump methods.

Some ByteBuffers can be converted to a byte[] without the penalty of a copy operation.
If performance becomes an issue then we can re-visit supporting ByteBuffer.


> 
> Comments below.
> 
> Paul.
> 
>> On 7 Dec 2016, at 08:32, Vincent Ryan <vincent.x.ryan at oracle.com> wrote:
>> 
>> A hexdump facility has been available for many, many years via an unsupported class: sun.misc.HexDumpEncoder.
>> Although that class was always unsupported, it was still accessible. That accessibility changes with Jigsaw so I’m proposing
>> a very simple replacement in a new and supported class: java.util.HexDump.
>> 
>> Thanks.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.00/
>> 
> 
>  55         if (bytes == null) {
>  56             throw new NullPointerException();
>  57         }
> 
> Objects.requireNonNull

OK

> 
> 
>  78      * @throws IllegalArgumentException if {@code hexString} has an odd number
>  79      *         of digits
>  80      * @throws NullPointerException if {@code hexString} is {@code null}
>  81      */
>  82     public static byte[] fromHexString(String hexString) {
> 
> This should accept a CharSequence, plus also have bounded range equivalent.

toHexString and fromHexString are expected to operate on reasonably short byte arrays.

I have added range equivalents for dump and dumpToStream where longer byte arrays
are expected.


> 
> Also throws IAE if the hexString contains non-convertible characters.
> 

OK

> 
> 106     /**
> 107      * Generates a stream of hexadecimal strings, in 16-byte chunks,
> 108      * from the contents of a binary buffer.
> 109      *
> 110      * @param bytes a binary buffer
> 111      * @return a stream of hexadecimal strings
> 112      * @throws NullPointerException if {@code bytes} is {@code null}
> 113      */
> 114     public static Stream<String> dumpToStream(byte[] bytes) {
> 115         if (bytes == null) {
> 116             throw new NullPointerException();
> 117         }
> 118         return StreamSupport.stream(
> 119             Spliterators.spliteratorUnknownSize(
> 120                 new HexDumpIterator(bytes),
> 121                 Spliterator.ORDERED | Spliterator.NONNULL),
> 122             false);
> 123     }
> 
> I suspect there is no need for a HexDumpIterator and you can do:
> 
>  return IntStream.range(0, roundUp(bytes.length, 16)).mapToObject(…);
> 
> and in the map function take care of the trailing bytes based on the byte[] length. That’s potentially simple enough there might be no need for a stream  method. Where it gets more complicated is if the source is of unknown size such as InputStream, where the stream returning method probably has more value.
> 
> Ideally what we want to return is Stream<[Long, String]>, then it’s really easy for others for format, but alas the current form would be ugly and inefficient. So i suspect the dump method has some legs but it’s real value may be for a fully streaming solution.
> 
> 
> 140     public static String dump(byte[] bytes) {
> 141         if (bytes == null) {
> 142             throw new NullPointerException();
> 143         }
> 144
> 145         int[] count = { -16 };
> 146         return dumpToStream(bytes).collect(Collector.of(
> 147             StringBuilder::new,
> 148                 (stringBuilder, hexString) ->
> 149                     stringBuilder
> 150                         .append(String.format("%08x  %s%n",
> 151                             (count[0] += CHUNK_LENGTH),
> 152                             explode(hexString))),
> 153             StringBuilder::append,
> 154             StringBuilder::toString));
> 155     }
> 
> The encoding of the count and exploding could also append directly into the main string builder, reducing memory churn.
> 
> And i think you can also start from IntStream.range(0, roundUp(bytes.length, 16)) avoiding the count state.

Thanks for those suggested improvements. They look good but I'd prefer to make implementation changes in a follow-up.


> 
> 
> 192             if (b < ' ' || b > '~') {
> 193                 sb.append(".”);
> 
> Use ‘.’

OK

> 
> 
> 194             } else {
> 195                 sb.append(new String(new byte[]{ b }));
> 
> Cast the byte to a char instead.

OK

> 
> 
> 196             }
> 
> 
> 
> 
> 
> 
> 
> 
>