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 nam
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 arr
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 thi
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,
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.
Regards, Peter
On 5/17/19 8:45 PM, Ivan Gerasimov wrote:
Hi Roger!
I think that not only the name, but also t
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 b
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 c
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')
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 u
Thanks for updating your patch. The updated code seems fine.
-Pavel
> On 11 May 2019, at 05:01, Ivan Gerasimov 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/
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()
Thank you Pavel for the investigation!
I've seen some of these places, and decided not to touch them with this
refactoring for various reasons:
java.lang.AbstractStringBuilder - the case has additional complication
due to interference of coder. It may make sense to refactor it later
within
Thank you Roger so much for valuable feedback!
On 5/10/19 7:41 AM, Roger Riggs wrote:
Hi Ivan,
Thanks for refactoring[1] this sensitive function.
ArraySupport.java:
Line 33: Please add a period at the end of the sentence.
I would have added a new sentence instead of mixing functions.
Don
Hi Peter!
Thank you for reviewing!
On 5/10/19 1:52 AM, Peter Levart wrote:
Hi Ivan,
On 5/9/19 8:07 PM, Ivan Gerasimov wrote:
3. I know that this is not new and has been copied from the old
code. However,
I'm not sure I understand the meaning of "unless necessary" here:
/**
* The
> On 10 May 2019, at 15:41, Roger Riggs wrote:
>
> 98: I think you've dropped the normal doubling of the buffer size that comes
> from old line 115.
> The buffer should be doubling in size, but at least minsize.
I thought about it too initially. But then I reproduced that function. It seemed
Hi Ivan,
Thanks for refactoring[1] this sensitive function.
ArraySupport.java:
Line 33: Please add a period at the end of the sentence.
I would have added a new sentence instead of mixing functions.
Line 583: Making MAX_ARRAY_SIZE public would make it accessible within
java.base module.
L
Ivan,
There seem to be more places that use a somewhat similar pattern. I was
wondering if you have seen them and decided not to include them in your patch
for some reason (e.g. they really are quite different)?
Here are some of them:
java.io.BufferedInputStream
java.io.InputStream
j
> On 10 May 2019, at 09:52, Peter Levart wrote:
>
>
>
> Is there a case where returning > MAX_ARRAY_SIZE will not lead to OOME?
>
> If this utility method is meant for re-sizing arrays only (currently it is
> only used for that), then perhaps the method could throw OOME explicitly in
> this
Hi Ivan,
On 5/9/19 8:07 PM, Ivan Gerasimov wrote:
3. I know that this is not new and has been copied from the old code.
However,
I'm not sure I understand the meaning of "unless necessary" here:
/**
* The maximum size of array to allocate (unless necessary).
It means that if the min
History: I was responsible for some of these. I considered refactoring but
did not because
- this was pre-module so no good way to share infrastructure.
- growth algorithm differed slightly between instances (looks like you've
solved this)
I agree this code is errorprone.
I might have more time
Thank you Pavel for careful review.
On 5/9/19 5:09 AM, Pavel Rappo wrote:
Ivan,
Many thanks for doing this! This code is both error-prone and abundant in the
JDK, which indeed makes it a perfect candidate for refactoring.
Since that particular change touches quite a few foundational parts of
Ivan,
Many thanks for doing this! This code is both error-prone and abundant in the
JDK, which indeed makes it a perfect candidate for refactoring.
Since that particular change touches quite a few foundational parts of the
libraries I felt the urge to make sure the change doesn't introduce any
ar
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 calcula
23 matches
Mail list logo
