Hi Roger, new webrev: http://cr.openjdk.java.net/~goetz/wr18/8201593-lenInAIOOB/07/
See my comments inline. > -----Original Message----- > From: Roger Riggs [mailto:roger.ri...@oracle.com] > > 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). ok, I removed the "." > I'm not a big fan of the hyphens in out-of-bounds and it would be more > consistent across the new messages. I'll do a follow-up and remove the hyphens everywhere. Also removed here. > It would be more consistent if the "arraycopy:" prefix always included > the ":". Added. Best regards, Goetz. > > 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:si...@cjnash.com] > >> Sent: Dienstag, 24. April 2018 18:17 > >> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com> > >> Cc: David Holmes <david.hol...@oracle.com>; hotspot-runtime- > >> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > aarch64- > >> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; core- > libs- > >> dev Libs <core-libs-dev@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.hol...@oracle.com] > >>>> Sent: Freitag, 20. April 2018 09:25 > >>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; hotspot- > runtime- > >>>> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > >> aarch64- > >>>> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; > core- > >> libs- > >>>> dev Libs <core-libs-dev@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.hol...@oracle.com] > >>>>>> Sent: Mittwoch, 18. April 2018 10:55 > >>>>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; hotspot- > >> runtime- > >>>>>> d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; > >>>> aarch64- > >>>>>> port-...@openjdk.java.net; aarch32-port-...@openjdk.java.net; > >> core- > >>>> libs- > >>>>>> dev Libs <core-libs-dev@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. > >>>>>>> > >>>>>>>
