Thanks for your review comments Paul. Responses below.
> On 8 Dec 2016, at 20:08, Paul Sandoz <paul.san...@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.r...@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 } > > > > > > > > >
