RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

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.

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
>>>
>>
>
>

-- 
With kind regards,
Ivan Gerasimov