RFR [8020669] java.nio.file.Files.readAllBytes() does not read any data when Files.size() is 0

[Ivan Gerasimov]

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.