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 >
