RFR: 8170769 Provide a simple hexdump facility for binary data
Alan.Bateman at oracle.com
Tue Dec 11 13:04:24 UTC 2018
On 08/12/2018 01:18, Vincent Ryan wrote:
> Here’s the latest version that addresses all the current open issues:
> webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ <http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/>
> javadoc: http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html <http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html>
> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 <https://bugs.openjdk.java.net/browse/CCC-8170769>
I read through the latest javadoc corresponding to webrev.07.
Overall I think it looks very good except for dumpAsStream(ByteBuffer,
fromIndex, toIndex, chunkSize, Formatter) as it's not clear if fromIndex
is from the buffer position or an absolute index. If the former then
there are a couple of other issues. I see Roger has touched on the
advancement of the buffer position to its limit which isn't right unless
all remaining bytes in the buffer are consumer. There is also an issue
with the exception as attempting to consume beyond the limit is a
BufferUnderflowException. It might be simpler to replace this method
with a dumpAsStream(ByteBuffer, chunkSize, Formatter) that can lazily
(rather than eagerly) consume the remaining bytes. Additional overloads
could be added in the future if needed.
dumpAsStream(InputStream) specifies "On return, the input stream will be
at end-of-stream" but I assume this isn't right as the method is lazy.
The 3-arg dumpAsStream doesn't specify the exception/behavior for when
chunkSize is <= 0.
fromString(CharSequence) may need clarification on how it behaves when
the CS is a CharBuffer. Does it consume all the remaining chars in the
buffer so its position be advanced to the limit? The 3-arg version is
also not clear on this point.
In Formatter interface it may be useful to expand the description of the
"offset" parameter as readers may not immediately understand that it's
just an input for the formatted string rather than a real offset, an
example might help. It also doesn't say if the offset can be specified
as <= 0L or not.
A minor comment is that several places refer to a byte array as a "byte
buffers. Is this deliberate or can these be replaced with "byte array"
to avoid confusion. Another minor comment is that one of the
dumpAsStream methods is missing @throws NPE - maybe they should all be
removed and replaced with a statement in the class description.
More information about the core-libs-dev