On 25/07/2013 16:56, Ivan Gerasimov wrote:
Would you please help review an updated webrev?
http://cr.openjdk.java.net/~igerasim/8020669/4/webrev/
Sorry for the late response to your mails on this, I was on vacation.
As you have found, in the original implementation we allowed for
resizing (truncation or extending) while the file was open but when it
was optimized to avoid an unnecessary resize of the array then we lost
the ability to deal with extending case (which is the essentially the
same as when the size is reported is less than the actual size).
Reverting to the original implementation and doing a single read to
double check for EOF seems reasonable.
My only real comment on the latest webrev (4) is that the computation of
the new capacity is not obvious to the reader (and since this is the
uncommon case then it doesn't need to be super-optimized). So I think
it would be a lot clearer if "newCapacity" were re-introduced and then
computed to a value that is a minimum of capacity+2, maximum of
capacity+BUFFER_SIZE (with OOME handling when newCapacity is greater
than MAX_BUFFER_SIZE). I think that would be clearer than re-using the
remaining count (rem).
A minor comment is that the input stream is named "fis" but probably
should be "in" as it is not a FileInputStream (I see David Llyod
suggested using a FileInputStream but the path may be to a file in a
custom file system so it can't be a FileInputStream).
I see you've changed the javadoc from "Read" to "Reads" on several
methods. I don't know if you meant to include this in the webrev but
personally prefer "Read".
Thanks for moving the test to BytesAndLines as that is the test for
these methods. One suggestion is to move the new test for readAllBytes
into testReadAndWriteBytes and the new test for readAllLines into
testReadLines. It doesn't matter of course, it just keeps them together.
-Alan.
