Re: Please review: surrogate fiddle

2013-03-26 Thread Martin Buchholz
On Wed, Mar 20, 2013 at 9:46 PM, Masayoshi Okutsu < masayoshi.oku...@oracle.com> wrote: > This fix is fine with me. > Submitted.

Re: Please review: surrogate fiddle

2013-03-21 Thread Martin Buchholz
On Thu, Mar 21, 2013 at 5:56 AM, Ulf Zibis wrote: > Anyway @throws would be better than @exception > > A JDK-wide cleanup of @exception => @throws (any many similar things) would be valuable, but I am not going to try here.

Re: Please review: surrogate fiddle

2013-03-21 Thread Martin Buchholz
On Thu, Mar 21, 2013 at 5:29 AM, Ulf Zibis wrote: > > Thanks! > But what's about the "wrong" exception index reporting, e.g. 0 as "out of > bounds"? > > I am not going to change the exceptions thrown from these methods, even though I agree we could do a little better if this was new code.

Re: Please review: surrogate fiddle

2013-03-21 Thread Ulf Zibis
Am 21.03.2013 14:09, schrieb Ulf Zibis: Yes, difficult to decide, but keep in the back of your mind, it's called _String_IndexOutOfBoundsException, not _CodepointAt_IndexOutOfBoundsException Oops, I meant ... _CodePointBefore_IndexOutOfBoundsException

Re: Please review: surrogate fiddle

2013-03-21 Thread Ulf Zibis
Am 20.03.2013 16:00, schrieb Alexander Zuev: AbstractStringBuilder: Instead 270 public int codePointBefore(int index) { 271 int i = index - 1; 272 if ((i < 0) || (i >= count)) { 273 throw new StringIndexOutOfBoundsException(index); 274 } I suggest 270 public int codePointBefore(int index) { 271 i

Re: Please review: surrogate fiddle

2013-03-21 Thread Ulf Zibis
Am 21.03.2013 00:22, schrieb Martin Buchholz: AbstractStringBuilder: Instead 270 public int codePointBefore(int index) { 271 int i = index - 1; 272 if ((i < 0) || (i >= count)) { 273 throw new StringIndexOutOfBoundsException(index); 2

Re: Please review: surrogate fiddle

2013-03-21 Thread Ulf Zibis
Am 21.03.2013 00:22, schrieb Martin Buchholz: On Wed, Mar 20, 2013 at 7:09 AM, Ulf Zibis mailto:ulf.zi...@cosoco.de>> wrote: AbstractStringBuilder: Instead 270 public int codePointBefore(int index) { 271 int i = index - 1; 272 if ((i < 0) || (i >= count

Re: Please review: surrogate fiddle

2013-03-20 Thread Martin Buchholz
On Mon, Mar 18, 2013 at 11:10 PM, Masayoshi Okutsu < masayoshi.oku...@oracle.com> wrote: > On 3/19/2013 3:04 PM, Martin Buchholz wrote: > > > On Mon, Mar 18, 2013 at 7:44 PM, Masayoshi Okutsu < > masayoshi.oku...@oracle.com> wrote: > >> As for duplicating code, I originally duplicated similar cod

Re: Please review: surrogate fiddle

2013-03-20 Thread Martin Buchholz
On Wed, Mar 20, 2013 at 7:09 AM, Ulf Zibis wrote: > Hi Martin, > > nice to see you again on board. > > Hi Ulf! > Am 19.03.2013 20:18, schrieb Martin Buchholz: > >> Thanks! Webrev updated. >> > > Character: > Maybe I'm blind, is there any semantical difference between > char c1 = seq.ch

Re: Please review: surrogate fiddle

2013-03-20 Thread Alexander Zuev
Hi Ulf, please see my comments inline. On 3/20/13 18:09, Ulf Zibis wrote: Hi Martin, nice to see you again on board. Am 19.03.2013 20:18, schrieb Martin Buchholz: Thanks! Webrev updated. Character: Maybe I'm blind, is there any semantical difference between char c1 = seq.charAt(in

Re: Please review: surrogate fiddle

2013-03-20 Thread Ulf Zibis
Hi Martin, nice to see you again on board. Am 19.03.2013 20:18, schrieb Martin Buchholz: Thanks! Webrev updated. Character: Maybe I'm blind, is there any semantical difference between char c1 = seq.charAt(index++); if (isHighSurrogate(c1)) { if (index < seq.length

Re: Please review: surrogate fiddle

2013-03-19 Thread Martin Buchholz
I couldn't resist fixing the two typos of ArrayIndexOutofBoundsException -// throws ArrayIndexOutofBoundsException if index out of bounds +// throws ArrayIndexOutOfBoundsException if index out of bounds

Re: Please review: surrogate fiddle

2013-03-19 Thread Martin Buchholz
Thanks! Webrev updated. On Tue, Mar 19, 2013 at 11:58 AM, Xueming Shen wrote: > ** > On 03/19/2013 11:49 AM, Martin Buchholz wrote: > > > Masayoshi or Xueming, please file a bug for me: > Title: Improve handling of char sequences containing surrogates > Desc: Fix and optimize codePointAt, codeP

Re: Please review: surrogate fiddle

2013-03-19 Thread Xueming Shen
On 03/19/2013 11:49 AM, Martin Buchholz wrote: Masayoshi or Xueming, please file a bug for me: Title: Improve handling of char sequences containing surrogates Desc: Fix and optimize codePointAt, codePointBefore and similar methods Eval: A fine idea! JDK-8010316: Improve handling of char sequenc

Re: Please review: surrogate fiddle

2013-03-19 Thread Martin Buchholz
Masayoshi or Xueming, please file a bug for me: Title: Improve handling of char sequences containing surrogates Desc: Fix and optimize codePointAt, codePointBefore and similar methods Eval: A fine idea!

Re: Please review: surrogate fiddle

2013-03-19 Thread Martin Buchholz
On Mon, Mar 18, 2013 at 11:10 PM, Masayoshi Okutsu < masayoshi.oku...@oracle.com> wrote: > > For classes as important as important as StringBuilder, I think we > should go the extra mile to ensure best performance, > > > That was exactly the reason why I initially took the same approach. So, > I'

Re: Please review: surrogate fiddle

2013-03-18 Thread Martin Buchholz
On Mon, Mar 18, 2013 at 7:44 PM, Masayoshi Okutsu < masayoshi.oku...@oracle.com> wrote: > Thank you for catching this bug. AbstractStringBuilder.**codePointAt(int) > should have called Character.codePointAt(char[], int, int). > > As for duplicating code, I originally duplicated similar code everyw

Re: Please review: surrogate fiddle

2013-03-18 Thread Martin Buchholz
On Mon, Mar 18, 2013 at 3:37 PM, Xueming Shen wrote: > ** > > The spec of both affected methods says "the char at the index is returned, > if > the following conditions are not true > > 1) hi-surrogate && > 2) next index < count && > 3) char at next is lo-surrogate > > So it is the right thing to

Re: Please review: surrogate fiddle

2013-03-18 Thread Masayoshi Okutsu
Thank you for catching this bug. AbstractStringBuilder.codePointAt(int) should have called Character.codePointAt(char[], int, int). As for duplicating code, I originally duplicated similar code everywhere for performance. But someone told me probably during code review that hotspot inlining wa

Re: Please review: surrogate fiddle

2013-03-18 Thread Xueming Shen
The spec of both affected methods says "the char at the index is returned, if the following conditions are not true 1) hi-surrogate && 2) next index < count && 3) char at next is lo-surrogate So it is the right thing to do to simply return the char at index, if it is the last char in the buffe

Re: Please review: surrogate fiddle

2013-03-18 Thread Martin Buchholz
It does change the behavior. The existing behavior is clearly a bug, since it reads a char that should be inaccessible. I don't believe AIOOBE exception is thrown, with or without my fix. On Mon, Mar 18, 2013 at 3:08 PM, Mike Duigou wrote: > This change would seem to change the result when a

Re: Please review: surrogate fiddle

2013-03-18 Thread Mike Duigou
This change would seem to change the result when a high surrogate is the last char in the String/StringBuilder/StringBuffer. Rather than throwing an ArrayIndexException it will return the high surrogate char. I am going to defer to Sherman on this. I don't know that returning the character is

Please review: surrogate fiddle

2013-03-18 Thread Martin Buchholz
Hello Jim, Mike, I'd like you to do a code review: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/surrogate-fiddle/