Hello Alan and everyone else
Thank you for review!
Here's another version of webrev:
http://cr.openjdk.java.net/~igerasim/8020669/5/webrev/
<http://cr.openjdk.java.net/%7Eigerasim/8020669/5/webrev/>
The comments are inline.
On 26.07.2013 8:54, Alan Bateman wrote:
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).
OK. I simplified it once again.
Now the new capacity is set to one of {BUFFER_SIZE, 2 * old capacity,
MAX_BUFFER_SIZE}, whatever is appropriate.
I think that doubling the capacity is better than increasing it by
BUFFER_SIZE, since this will allow to allocate needed buffer in less
number of iterations.
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).
Renamed.
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".
Files.java has mixed style. Most methods are documented with "Does"
form, but some have "Do".
I left "Reads" on the methods I modify for consistency, but reverted
changes from other places as I'm not going to change all the occurrences
in this webrev.
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.
Sure, done.
Sincerely yours,
Ivan
-Alan.
