RFR: 8071571: Move substring of same string to slow path

[Martin Buchholz]

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