codereview request: 6639443/

[Ulf Zibis]

Sherman, Martin,

Why you don't commit this change to the repository ?

Especially please add Character.isSurrogate(char ch) soon.
I like to get rid of using the sun.nio.cs.Surrogate class in my changes.

IMO, there only should remain Surrogate.Parser and Surrogate.Generator, 
if someone likes to use it.

-Ulf



Am 14.07.2009 21:50, Xueming Shen schrieb:
> I included the core-libs-dev as you suggested.
>
>
>
> (1)The toCodePoint change looks good.
>
> (2)
>
> a)    return (int)(char) uc ==uc;
>
>    is nice:-) but I would go with the "more easy to read"
>
>    return uc < Surrogate.UCS4_MIN;
>
>    if it were my code. Is there a big performance gain by doing that?
>
> b) The "buffer" version and the "array" version of generate() are not 
> synced.
>
>
> (3)6860431: Character.isSurrogate(char ch)
>
> has been filed on your behalf, as well as the CCC 
> http://ccc.sfbay.sun.com/6860431.
> Masayoshi, would you please review the spec and add yourself as the 
> reviewer? I
> need the reviewer to fast-track it.
>
> Sherman
>
> Martin Buchholz wrote:
>> Hey Character team,
>>
>> (should we add more people to this conversation?)
>>
>> One of the reasons I don't finish projects is
>> that I never think the code is good enough.
>> In that spirit, here are 3 reviews for you:
>>
>> ----
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/toCodePoint/ 
>> <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/toCodePoint/>
>>
>> Since the unoptimized code is easier to understand,
>> I re-added it, in a comment.
>>
>> ----
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Surrogate/ 
>> <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/Surrogate/>
>>
>> I removed more (but certainly not all) uses of Surrogate
>> stuff that was duplicated from Character.
>>
>> I added a TODO to remove more such code duplication.
>>
>> In generate(), there is now a genuine difference in behavior.
>> I believe the intent of the original code was to handle
>> negative codepoint values, but never quite got that right.
>> My fix removed dead code, but was incorrect in that
>> I should have preserved the intent, not just behavior.
>>
>> I optimized for the common case of BMP chars,
>> and introduced isBMP.
>>
>> ----
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/isSurrogate/ 
>> <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/isSurrogate/>
>>
>> This is a spec change.  Please file bug and CCC on my behalf.
>> Synopsis: Character.isSurrogate(char ch)
>> Description:
>> Character.isSurrogate(char ch)
>> is *the* missing method
>> that we all use from Surrogate.java.
>>
>> Let's put it into Character.
>>
>> When approved, we can make sure it's tested by
>> replacing calls to Surrogate.is(int) with calls to
>> Character.isSurrogate(char) (but they're not exactly the same
>> because the domain of the function is different.
>>
>> ----
>>
>> Thanks,
>>
>> Martin
>>
>> On Fri, Jul 10, 2009 at 17:39, Martin Buchholz <martinrb at google.com 
>> <mailto:martinrb at google.com>> wrote:
>>
>>     Thanks for kicking me!
>>
>>     I have a bad habit of starting projects and not finishing them.
>>     If it's OK with you, I will commit this change (and perhaps others
>>     like it).  Let's call it guilt-driven software development.
>>
>>     Martin
>>
>>
>>     On Fri, Jul 10, 2009 at 16:13, Xueming Shen <Xueming.Shen at sun.com
>>     <mailto:Xueming.Shen at sun.com>> wrote:
>>
>>         I'm cleaning up the 7 bug list, this one is the one you did
>>         not finish/putback before left.
>>
>>         http://cr.openjdk.java.net/~sherman/6639443/webrev
>>         <http://cr.openjdk.java.net/%7Esherman/6639443/webrev>
>>
>>         The fix looks fine. Can you "review" it as well?
>>
>>         Masayoshi, please take a look as well.
>>
>>         Btw, where is the regression tests for supplementary
>>         characters? I can't find it under test/java/lang...
>>
>>         Thanks!
>>         Sherman
>>
>>
>>
>
>