JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart patrick at reini.net
Fri Nov 10 09:42:36 UTC 2017


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
> 



More information about the core-libs-dev mailing list