RFR 8223593 : Refactor code for reallocating storage

[Peter Levart]

On 5/20/19 11:11 AM, Ivan Gerasimov wrote:
> Hi Peter!
>
>
> On 5/19/19 11:17 PM, Peter Levart wrote:
>> Hi Ivan, Roger,
>>
>> What about "calcNewLength" ?
>>
>> The word "new" gives enough hint as to what the method does - it 
>> calculates the length of new array to be allocated instead of old one.
>>
> Yes, I think it was one of intermediate names.
> But then, it seemed that the New part is redundant, as it does not add 
> much:  "calculate length" and "calculate new length" sound very 
> similar, while the former is shorter.

I feel it does add a tiny bit to clarity. This is internal API, so it's 
not that important, and from its use it is immediately obvious what's 
going on, but even you felt that saying:

 > calcLength(oldLength, minGrowth, prefGrowth) is meant to be read as 
"given old length and the amount(s) to grow, calculate the new length"

is more clear than:

 > calcLength(oldLength, minGrowth, prefGrowth) is meant to be read as 
"given old length and the amount(s) to grow, calculate the length"

If a word is redundant then it is "calc". It is obvious that any 
function calculates something. So what about "newLength()" ?

Regards, Peter

>
> With kind regards,
> Ivan
>
>> Regards, Peter
>>
>> On 5/17/19 8:45 PM, Ivan Gerasimov wrote:
>>> Hi Roger!
>>>
>>> I think that not only the name, but also the arguments compose the 
>>> signature.
>>> So, calcLength(oldLength, minGrowth, prefGrowth) is meant to be read 
>>> as "given old length and the amount(s) to grow, calculate the new 
>>> length".
>>> And since this method is placed into ArraysSupport, it should be 
>>> clear that it is an array's length.
>>>
>>> I understand that in certain contexts size may sound more natural 
>>> than length.
>>> Here, however, using 'length' should be a hint that we're dealing 
>>> with array.length.
>>>
>>> With kind regards,
>>> Ivan
>>>
>>> On 5/17/19 10:35 AM, Roger Riggs wrote:
>>>> Hi Ivan,
>>>>
>>>> The new calcLength method name is too generic, it does not say 
>>>> enough about its function.
>>>> There is no indication that the purpose is to resize an array.
>>>> As for size vs length, sometime size is more evocative of the 
>>>> function being performed
>>>> than 'length' and is more natural.  In most cases, size and length 
>>>> are understood to be synonyms
>>>> and capacity is a very apt term for how many elements can be held.
>>>>
>>>> Can I recommend resizedLength.
>>>>
>>>> Regards, Roger
>>>>
>>>>
>>>>
>>>> On 05/15/2019 10:47 PM, Ivan Gerasimov wrote:
>>>>> Thank you Pavel and Roger for reviewing!
>>>>>
>>>>> I apologize for reiterating this.
>>>>> After off-line discussion with Stuart Marks, the fix was modified 
>>>>> once again.
>>>> !!
>>>>> The modifications were mostly stylistic:  The used terminology now 
>>>>> reflects that we work with arrays (thus 'length', not 'size' or 
>>>>> 'capacity').
>>>>> Functionally, the fix remains exactly the same.
>>>>>
>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8223593
>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8223593/02/webrev/
>>>>>
>>>>> With kind regards,
>>>>> Ivan
>>>>>
>>>>> On 5/14/19 7:50 AM, Roger Riggs wrote:
>>>>>> Hi Ivan,
>>>>>>
>>>>>> The updated patch looks fine.
>>>>>>
>>>>>> Strictly speaking, the change to Files.readAllBytes is not 
>>>>>> indicated by the bug report
>>>>>> so please update or comment on the bug report to mention that 
>>>>>> change.
>>>>>>
>>>>>> Thanks, Roger
>>>>>>
>>>>>>
>>>>>> On 05/13/2019 10:44 AM, Pavel Rappo wrote:
>>>>>>> Thanks for updating your patch. The updated code seems fine.
>>>>>>>
>>>>>>> -Pavel
>>>>>>>
>>>>>>>> On 11 May 2019, at 05:01, Ivan 
>>>>>>>> Gerasimov<ivan.gerasimov at oracle.com>  wrote:
>>>>>>>>
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> Please help review the updated fix.
>>>>>>>>
>>>>>>>> This new webrev includes changes suggested by Pavel, Peter and 
>>>>>>>> Roger.
>>>>>>>>
>>>>>>>> BUGURL:https://bugs.openjdk.java.net/browse/JDK-8223593
>>>>>>>> WEBREV:http://cr.openjdk.java.net/~igerasim/8223593/01/webrev/
>>>>>>>>
>>>>>>>> Please note that the behavior of j.n.f.Files.readAllBytes() has 
>>>>>>>> changed slightly, so now it *may* be possible to read a file 
>>>>>>>> larger than (Integer.MAX_VALUE - 8), if VM is able to allocate 
>>>>>>>> that large array.
>>>>>>>>
>>>>>>>> With kind regards,
>>>>>>>> Ivan
>>>>>>>>
>>>>>>>> On 5/8/19 6:50 PM, Ivan Gerasimov wrote:
>>>>>>>>> Hello!
>>>>>>>>>
>>>>>>>>> Jdk has several places with similar logic:  an array needs to 
>>>>>>>>> be reallocated (by at least some defined amount), taking into 
>>>>>>>>> account the maximum allowed size of arrays.
>>>>>>>>>
>>>>>>>>> There's clearly an opportunity for refactoring, so it is 
>>>>>>>>> proposed to introduce a dedicated utility method for 
>>>>>>>>> calculating the best new size of an array.
>>>>>>>>>
>>>>>>>>> Would you please help review this enhancement?
>>>>>>>>>
>>>>>>>>> BUGURL:https://bugs.openjdk.java.net/browse/JDK-8223593
>>>>>>>>> WEBREV:http://cr.openjdk.java.net/~igerasim/8223593/00/webrev/
>>>>>>>>>
>>>>>>>>> Mach5 job ran fine.
>>>>>>>>>
>>>>>>>>> Thanks in advance!
>>>>>>>>>
>>>>>>>> -- 
>>>>>>>> With kind regards,
>>>>>>>> Ivan Gerasimov
>>>>>>>>
>>>>>>
>>>>>
>>>>> -- 
>>>>> With kind regards,
>>>>> Ivan Gerasimov
>>>>
>>>
>>
>>
>