+1 Paul.
> On Jun 2, 2020, at 6:21 AM, Jim Laskey <james.las...@oracle.com> wrote: > > Revised with requested changes. > > > http://cr.openjdk.java.net/~jlaskey/8230743/webrev-01 > <http://cr.openjdk.java.net/~jlaskey/8230743/webrev-01> > > >> On Jun 1, 2020, at 5:32 PM, Paul Sandoz <paul.san...@oracle.com> wrote: >> >> >> >>> On Jun 1, 2020, at 12:44 PM, Jim Laskey <james.las...@oracle.com >>> <mailto:james.las...@oracle.com>> wrote: >>> >>> >>> >>>> On Jun 1, 2020, at 4:28 PM, Paul Sandoz <paul.san...@oracle.com >>>> <mailto:paul.san...@oracle.com>> wrote: >>>> >>>> Can we consolidate the use of Integer.MAX_VALUE - 8 in tests by referring >>>> to the constant jdk.internal.util.ArraysSupport.MAX_ARRAY_LENGTH? >>> >>> I suppose it's possible to add module for >>> java.base/jdk.internal.util.ArraysSupport to the tests. >>> >>> The mission here was to provide useful error messages. If we're going to >>> clean up growing arrays throughout the JDK, I feel that is a separate >>> mission, probably not something that we should be doing just before RD1. >>> >> >> Ok. >> >> >>> We should probably have gone one step further with >>> jdk.internal.util.ArraysSupport.newLength and just defined >>> jdk.internal.util.ArraysSupport.growArray(array, newSize, () -> { // >>> exception code }); >> >>> >>>> >>>> >>>> StringJoiner.java >>>> — >>>> >>>> 164 @Override >>>> 165 public String toString() { >>>> 166 final String[] elts = this.elts; >>>> 167 if (elts == null && emptyValue != null) { >>>> 168 return emptyValue; >>>> 169 } >>>> 170 final int size = this.size; >>>> 171 final int addLen = prefix.length() + suffix.length(); >>>> 172 if (len < 0 || len + addLen < 0) { >>>> >>>> It bothers me that len might be < 0, suggesting a larger issue. Perhaps >>>> look at the add method where len is modified? >>> >>> I thought about it but that would change the behaviour of StringJoiner. An >>> error would be thrown on the add. Existing code that added but then tested >>> length before trying a toString() would fail too soon. >> >> Or fail at the right point? Lazy String construction by the joiner can be >> considered an implementation detail. >> >> A negative len could result in some odd behavior: >> >> - an incorrect value returned from length(), such as a negative length value. >> >> - a number of adds could be performed, which one would have resulted in an >> OOME if the implementation was not lazy? >> >> - (len + other.len < 0) could be positive and result in array construction >> exception for a negative length on other.compactElts. >> >> Paul. >> >>> >>>> >>>> >>>> 173 throw new >>>> 174 OutOfMemoryError("Resulting string is exceeds maximum >>>> size”); >>>> >>>> "Resulting string is exceeds… ” -> "Resulting string exceeds… " >>> >>> Oops, thanks. >>> >>> >>>> >>>> Paul. >>>> >>>> >>>>> On Jun 1, 2020, at 8:55 AM, Jim Laskey <james.las...@oracle.com> wrote: >>>>> >>>>> Change NegativeArraySizeException to OutOfMemoryError. Tests added. >>>>> >>>>> Cheers, >>>>> >>>>> -- Jim >>>>> >>>>> webrev: http://cr.openjdk.java.net/~jlaskey/8230743/webrev-00/index.html >>>>> <http://cr.openjdk.java.net/~jlaskey/8230743/webrev-00/index.html> >>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8230743 >>>>> <https://bugs.openjdk.java.net/browse/JDK-8230743> >
