On 01/20/2013 04:54 AM, Steven Schlansker wrote:
>> a) Do you need the masks before or-ing with most/leastSigBits?
>
> So, they seem to exist to satisfy a few Apache Harmony test cases:
> http://svn.apache.org/viewvc/harmony/enhanced/java/branches/java6/classlib/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/UUIDTest.java?revision=929252
I'm sure some due dilignence will be required to actually do this... but
can we add the similar-looking test to OpenJDK?
> For example,
> uuid = UUID.fromString("123456789-0-0-0-0");
> Assert.assertEquals(0x2345678900000000L, uuid.getMostSignificantBits());
> Assert.assertEquals(0x0L, uuid.getLeastSignificantBits());
> Without the masking, the values can smear together. It would be
> possible to avoid converting extra characters, but I believe that the
> mask is probably cheaper than an if to check for the presence of
> extra characters.
Yeah, OK, masking is better. Any specific reason some masks are integer,
not long?
> Were I writing the code from scratch I would throw an
> IllegalArgumentException on such a UUID but I assume that is not
> acceptable for compatibility reasons.
It is a gray area when exceptional behavior are considered. The changes
potentially breaking compatibility are better to be submitted as another
CR, not to drag the precious changes along.
>> c) I'd go for making utility methods a bit more generic. For one, I
>> would rather make decodeHex(String str, int start, int end), and
>> encodeHex(char[] dest, int offset, int value).
>
> It's not a huge difference either way, but I'm not sure that your suggestion
> is better?
My inclination was to match usual ($something, offset, count) or
($something, start, end) signatures you will see across the JDK. But in
this concrete example this seems to be a tie. Your code is as fine.
> I suppose I could make it non-private helper method available to
> others, although I am not sure if this is considered a good practice
> in the JDK code. Let me know if you think I should do this.
I think you should consider doing that, although it is not strictly
speaking required, especially given this pretty limited change. Anyway,
we have the sun.misc.* namespace for these helpers. You would be
surprised to see how many usages these helpers will bear once you have
them, and let some time to pass.
-Aleksey.
