RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

Hi Peter!


On 5/20/19 3:14 AM, Peter Levart wrote:
>
>
> 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()" ?
>
:-)
I'm fine with newLength, as it's even shorter than calcLength.

I agree with you that it's obvious from its usage what it's doing. And 
the javadoc of this method and comments for the arguments make it 
absolutely clear.

I can rename it before pushing, if I hear no other objections.

With kind regards,
Ivan

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

-- 
With kind regards,
Ivan Gerasimov