RFR: 8071571: Move substring of same string to slow path (original) (raw)

Martin Buchholz martinrb at google.com
Tue May 12 21:22:00 UTC 2015


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.gerasimov at 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.gerasimov at 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



More information about the core-libs-dev mailing list