All your changes look good. But: --- Not your bug, but it looks like the below should instead be: throw new StringIndexOutOfBoundsException(beginIndex); Perhaps fix in a follow-on change.
1935 if (subLen < 0) { 1936 throw new StringIndexOutOfBoundsException(subLen); --- We seem to have clean progress, but for substring fast-path we can do a little better, I think, thus: public String substring(int beginIndex, int endIndex) { int subLen; return ((beginIndex | (value.length - endIndex)) > 0 && (subLen = endIndex - beginIndex) > 0) ? new String(value, beginIndex, subLen) : substringSlowPath(beginIndex, endIndex); } private String substringSlowPath(int beginIndex, int endIndex) { if (beginIndex <= 0) { if (beginIndex < 0) { throw new StringIndexOutOfBoundsException(beginIndex); } if (endIndex == value.length) { return this; } } if (endIndex > value.length) { throw new StringIndexOutOfBoundsException(endIndex); } if (endIndex == beginIndex) { return ""; } throw new StringIndexOutOfBoundsException(endIndex - beginIndex); } additional white-box tests would be required if we adopt that. On Tue, May 12, 2015 at 11:58 AM, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote: > > > On 12.05.2015 20:34, Martin Buchholz wrote: > > Hi Ivan, > > The code below looks wrong to me - sb.length() resolves to sb.count, not > v2.length. > If I'm correct, then there's a missing test to be added, since this error > should be caught by some test. > > private boolean nonSyncContentEquals(AbstractStringBuilder sb) { > - char v1[] = value; > - char v2[] = sb.getValue(); > + char[] v1 = value; > + char[] v2 = sb.getValue(); > int n = v1.length; > - if (n != sb.length()) { > + if (n != v2.length) { > return false; > } > > > Yes, of course you're right. This change looked so "obviously correct" > to me, that I didn't care to run the tests before posting the webrev :-( > > I've reverted this change back: > http://cr.openjdk.java.net/~igerasim/8071571/01/webrev/ > > Sincerely yours, > Ivan > > > On Mon, May 11, 2015 at 1:52 PM, Ivan Gerasimov <ivan.gerasi...@oracle.com > > wrote: > >> I have to take over this fix. >> >> The latest webrev from the review thread above (with a few minor changes) >> is here: >> http://cr.openjdk.java.net/~igerasim/8071571/00/webrev/ >> >> Would you please review/approve the fix at your convenience? >> >> Sincerely yours, >> Ivan >> >> > >