Hi Patrick,
Comments on http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.02:
Readable.java:
74: "to an error" is a bit overloaded, perhaps "to an exception"
X-Buffer.java.template:
1567: "this buffer": do you mean the "out buffer"
1578: "The out buffer is this buffer": will read poorly out of context,
perhaps: "Illegal transfer of a buffer to itself"
1580: Perhaps confine "start" to the else case.
For the tests, an example of using RandomFactory is in
test/jdk/java/io/InputStream/ReadNBytes.java.
Thanks, Roger
On 11/10/2017 4:42 AM, Patrick Reinhart wrote:
An updated webrev:
http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.01
A few comments:
Readable.java:
67: + it may be worth mentioning that the input might not fit in output (as
seen in the CharBuffer case)
Though I see we didn’t call that out in the other transferTo description but
here it is more likely that the output is bounded.
I tried to mention that now.
77: "The total amount will be added up by after the write method has been
completed." should not be part
of the @implSpec. It is untestable and unnecessary. If an exception occurs the
value is lost.
I see that point - removed
96: „in order not having the extra overhead creating" -> "to avoid the extra
overhead of"
done
X-Buffer.java.template:
1555: „form" -> "from"
done
1554: I would avoid most of the details in the @implSpec since it is requiring
a specific use of CharBuffer methods.
That's fine as an @ImplNote but restricts the implementation too much as spec.
(others will disagree)
1565: in the code, perhaps it should use an explicit RequireNonNull(out,
„out") otherwise the NPE will be on put()/append().
done
1566: "insufficient space in this" refers to the source, not dest and it should
only apply if out is a CharBuffer.
Omit it or leave it more general; that behavior is covered by the spec of
the out Appendable.
1568: I don’t see code to enforce: "if out is this buffer"
done
On the tests; had you considered using TestNG;
it provides some good structure and is preferred (AFAIK) for new tests.
To be honest, no. The test for the Readable I basically copied from the
InputStream and the one for CharBuffer I just did
not think about… I will certainly consider that for the future :-)
As for Randomness, it is useful to be explicit about the seed used in the
particular run so it can be
replayed if necessary. There is a testlibrary function to do that if you don't
want to code-your-own.
(test/jdk/test/lib/RandomFactory::getRandom())
I will need some digging how to have the RandomFactory be added to the test
class path…
Thanks, Roger
