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

Fri May 4 21:13:27 UTC 2018

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

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

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

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