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
- Previous message: RFR: 8071571: Move substring of same string to slow path
- Next message: RFR: 8071571: Move substring of same string to slow path
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
- Previous message: RFR: 8071571: Move substring of same string to slow path
- Next message: RFR: 8071571: Move substring of same string to slow path
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]