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 <marti...@google.com
<mailto:marti...@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.s...@sun.com
<mailto:xueming.s...@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
