codereview request: 6639443/

[Xueming Shen]

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