RFR 8223593 : Refactor code for reallocating storage

Mon May 20 16:59:06 UTC 2019

Hi,

I'm fine with newLength.  Thanks Peter for articulating what was missing 
from calcLength.

The method name needs to make a positive contribution to comprehension.
The three integer arguments don't contribute much, especially since they
are constants in some cases, and in other cases are named for the 
function they
perform in the callers context and are typically index related, not length.

Thanks, Roger

On 05/20/2019 06:45 AM, Ivan Gerasimov wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>