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>
