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