RFR(M): 8201593: Print array length in ArrayIndexOutOfBoundsException.

David Holmes david.holmes at oracle.com
Fri May 4 22:38:44 UTC 2018 

Not my final review just piggybacking on Roger's comments ...

On 5/05/2018 7:13 AM, Roger Riggs wrote:
> Hi Goetz,
> 
> Just comments on the test/message format.  (found in the 04 version of 
> the webrev)
> 
> The "." at the end of the exception messages should be removed.
> The text is not a sentence (its is just a fragment without a verb) and
> it is more flexible to print the exception message in various contexts
> (in which the "." would seem to terminate a sentence).

+1

> I'm not a big fan of the hyphens in out-of-bounds and it would be more 
> consistent across the new messages.

I also see hyphens used a lot in: java/util/Objects.java

But yes consistency within this patch is required.

> It would be more consistent if the "arraycopy:" prefix always included 
> the ":".

+1

David

> Thanks, Roger
> 
> 
> On 4/24/18 12:25 PM, Lindenmaier, Goetz wrote:
>> Hi Simon,
>>
>> Because as stated here,
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-April/052665.html 
>>
>> it is used in other places like that, too.
>>
>> Later mails agreed on that usage to keep it consistent.
>>
>> Best regards,
>>    Goetz.
>>
>>> -----Original Message-----
>>> From: Simon Nash [mailto:simon at cjnash.com]
>>> Sent: Dienstag, 24. April 2018 18:17
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>>> Cc: David Holmes <david.holmes at oracle.com>; hotspot-runtime-
>>> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net; aarch64-
>>> port-dev at openjdk.java.net; aarch32-port-dev at openjdk.java.net; core-libs-
>>> dev Libs <core-libs-dev at openjdk.java.net>
>>> Subject: Re: RFR(M): 8201593: Print array length in
>>> ArrayIndexOutOfBoundsException.
>>>
>>> On 24/04/2018 15:08, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> I implemented what we figured out in
>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-
>>> April/027555.html
>>>> Some examples:
>>>> "Index 12 out-of-bounds for length 10."
>>>> "arraycopy source index -17 out of bounds for object array[10]."
>>>> "arraycopy destination index -18 out of bounds for double[5]."
>>>> "arraycopy length -19 is negative."
>>>> "arraycopy: last source index 13 out of bounds for double[10]."
>>>> "arraycopy: last destination index 7 out of bounds for long[5]."
>>>>
>>> Is there a reason why the first message says "out-of-bounds" but all 
>>> others
>>> say "out of bounds"?
>>>
>>> Simon
>>>
>>>> Incremental webrev:
>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03-
>>> incremental/
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/03/
>>>>
>>>> I'll push it through our tests tonight.
>>>>
>>>> See further comments in line:
>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>> Sent: Freitag, 20. April 2018 09:25
>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>>>> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net;
>>> aarch64-
>>>>> port-dev at openjdk.java.net; aarch32-port-dev at openjdk.java.net; core-
>>> libs-
>>>>> dev Libs <core-libs-dev at openjdk.java.net>
>>>>> Subject: Re: RFR(M): 8201593: Print array length in
>>>>> ArrayIndexOutOfBoundsException.
>>>>>
>>>>> Hi Goetz,
>>>>>
>>>>> This is not a file by file review ...
>>>>>
>>>>> On 19/04/2018 10:24 PM, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> New webrev:
>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/02/
>>>>>>
>>>>>> I admit my wording is not optimal.
>>>>> src/hotspot/share/oops/typeArrayKlass.cpp
>>>>>
>>>>> Sorry but this is still far too verbose for my tastes. The type of the
>>>>> array is not relevant. For the array copy its okay to indicate src or
>>>>> dst array. But the message should be clear and succinct not 
>>>>> prose-like.
>>>>> Plus you have a lot of repetition in the ss.print statements when only
>>>>> one thing is changing.
>>>> We discussed this in some further mails. Implemented as proposed there.
>>>>
>>>>> src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp
>>>>>
>>>>> I'm not seeing why the throw_index_out_of_bounds_exception should
>>> be
>>>>> tied to whether or not you have an array reference. Certainly I hate
>>>>> seeing the array ref being used as an implicit bool!
>>>> I split the constructor into two, one for ArrayIndexOutOfBounds, the 
>>>> other
>>>> for IndexOutOfBounds.
>>>>
>>>> ...
>>>>
>>>>>> I extended the test to cover the exception thrown in arraycopy better
>>>>> The test seems somewhat excessive, and I now see it is because of the
>>>>> more elaborate error messages you have at SAP. But with only the index
>>>>> and the array length of interest here the test can be considerably 
>>>>> smaller.
>>>>>
>>>>> The creation tests for ArrayIndexOutOfBoundsException don't seem
>>>>> relevant in this context either. This looks more TCK like.
>>>> Yes, the constructor tests are for the code not yet contributed.
>>>> I simplified the tests to only check the messages.
>>>>
>>>> Best regards,
>>>>    Goetz.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> and added the elementary type to the message text.  This probably
>>>>>> needs improvement in the text, too.  There are (currently) these 
>>>>>> cases:
>>>>>>
>>>>>> bject[]  oa1 = new Object[10];
>>>>>> Object[]  oa2 = new Object[5];
>>>>>> System.arraycopy(oa1, -17, oa2, 0, 5);
>>>>>> "while trying to copy from index -17 of an object array with 
>>>>>> length 10");
>>>>>> System.arraycopy(oa1, 2, oa2, -18, 5);
>>>>>> "while trying to copy to index -18 of an object array with length 
>>>>>> 5");
>>>>>> System.arraycopy(oa1, 2, oa2, 0, -19);
>>>>>> "while trying to copy a negative range -19 from an object array with
>>> length
>>>>> 10 to an object array with length 5");
>>>>>> System.arraycopy(oa1, 8, oa2, 0, 5);
>>>>>> "while trying to copy from index 13 of an object array with length 
>>>>>> 10");
>>>>>> System.arraycopy(oa1, 1, oa2, 0, 7);
>>>>>> "while trying to copy to index 7 of an object array with length 5");
>>>>>> double[]  ta1 = new double[10];
>>>>>> double[]  ta2 = new double[5];
>>>>>> System.arraycopy(ta1, -17, ta2, 0, 5);
>>>>>> "while trying to copy from index -17 of a doubl array with length 
>>>>>> 10");
>>>>>> System.arraycopy(ta1, 2, ta2, -18, 5);
>>>>>> "while trying to copy to index -18 of a double array with length 5");
>>>>>> System.arraycopy(ta1, 2, ta2, 0, -19);
>>>>>> "while trying to copy a negative range -19 from a double array with
>>> length
>>>>> 10 to a double array with length 5");
>>>>>> System.arraycopy(ta1, 8, ta2, 0, 5);
>>>>>> "while trying to copy from index 13 of a double array with length 
>>>>>> 10");
>>>>>> System.arraycopy(ta1, 1, ta2, 0, 7);
>>>>>> "while trying to copy to index 7 of a double array with length 5");
>>>>>>
>>>>>> Maybe it should say:
>>>>>> Arraycopy source index -1 out-of-bounds for double array of length 
>>>>>> 10.
>>>>>> Arraycopy target index 10 out-of-bounds for object array of length 
>>>>>> 10.
>>>>>> Negative range -19 when copying from an object array of length 10 
>>>>>> to an
>>>>> object array of length 5.
>>>>>> Best regards,
>>>>>>     Goetz.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>>>> Sent: Mittwoch, 18. April 2018 10:55
>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>>> runtime-
>>>>>>> dev at openjdk.java.net; hotspot-compiler-dev at openjdk.java.net;
>>>>> aarch64-
>>>>>>> port-dev at openjdk.java.net; aarch32-port-dev at openjdk.java.net;
>>> core-
>>>>> libs-
>>>>>>> dev Libs <core-libs-dev at openjdk.java.net>
>>>>>>> Subject: Re: RFR(M): 8201593: Print array length in
>>>>>>> ArrayIndexOutOfBoundsException.
>>>>>>>
>>>>>>> Adding core-libs-dev as you're changing
>>>>>>> java.lang.ArrayIndexOutOfBoundsException.
>>>>>>>
>>>>>>> I appreciate the intent here but I find the messages excessively
>>>>>>> verbose. The basic error is:
>>>>>>>
>>>>>>> index N is outside range [0, length-1]
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On 18/04/2018 6:09 PM, Lindenmaier, Goetz wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I would like to print a more verbose text on ArrayIndexOutOfBounds
>>>>>>> exception
>>>>>>>> that not only mentions the index, but also the length of the array
>>>>> accessed.
>>>>>>>> See the bug for documentation of the change of the message.
>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/01/
>>>>>>>>
>>>>>>>> @aarch/arm people:
>>>>>>>> I edited the aarch/arm files. Could you please verify this is 
>>>>>>>> correct?
>>>>>>>> I can not build on these platforms.
>>>>>>>>
>>>>>>>> The code on all the other platforms is tested with all the jtreg 
>>>>>>>> and jck
>>>>> tests
>>>>>>> etc.
>>>>>>>> Best regards,
>>>>>>>>      Goetz.
>>>>>>>>
>>>>>>>>
>

More information about the core-libs-dev mailing list