RFR [8020669] java.nio.file.Files.readAllBytes() does not read any data when Files.size() is 0
Ivan Gerasimov
ivan.gerasimov at oracle.com
Sun Jul 28 15:08:58 PDT 2013
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20130729/2080f267/attachment.html
More information about the nio-dev
mailing list