[threeten-dev] Codereview request for 8003680: JSR 310: Date/Time API

[Xueming Shen]

On 01/16/2013 04:08 AM, Alan Bateman wrote:
> On 16/01/2013 00:13, Xueming Shen wrote:
>> Hi,
>>
>> The Threeten project [1] is planned to be integrated into OpenJDK8 M6 milestone.
>>
>> Here is the webrev
>> http://cr.openjdk.java.net/~sherman/8003680/webrev
>>
>> and the latest javadoc
>> http://cr.openjdk.java.net/~sherman/8003680/javadoc
>>
>> Review comments can be sent to the threeten-dev email list [2] and/or
>> core-libs-dev email list[3].
> This is not a review of the API or implementation. Rather just a few random comments as I look through the webrev.
>
> It looks to me that the zone rules compiler ends up in rt.jar, is that expected and is it actually used at runtime? On initial read I thought it was build-time only but I might be wrong. As per off-list discussion, it needs to run on the boot JDK to work in cross-compilation environments and so the dependency on java.time is an issue.


Yes, looking into rewriting or moving some pieces around to satisfy the cross-compilation env.
No, those should not be in rt.jar, at least for now.

>
> I see Formatter has been updated to support conversions of TemporalAccessor. Is the assert(width == -1) on L4067 right or was it copied from another print method? (Brian Burkhalter and I were chatting recently about an assert into another print method).

It's copy/pasted from print(StringBuffer, Calendar...)


> Also on Formatter then I think it would be good to put the tests in test/java/util/Formatter as someone touching the Formatter code might not know there are additional tests hiding down in the odd location test/java/time/test/java/util.
>

Yes, understood. The concern is that, at least for now, it is more likely we are
going to change the java/time and we forget there is another test case at
java/util/Formatter:-) Maybe it is more convenient to keep it here for a little
while until the threeten is stable enough. The "location" is odd, mainly to
satisfy the TestNG requirement (for tests with their own package). The test
is written to use TestNG.


> As you are adding a jdk_test target to test/Makefile then you will should also add the target to jprt.properties (btoh top-level repo and jdk/make). This is only interesting for Oracle build+test system of course.

Sure, will added.

>
> Just on the tests, is the plan to push the TCK tests to the jdk as proposed in the webrev?

Those tck tests also serve as unit tests. They are so named for the convenience
that TCK guys can just pick them up and drop into their suite easily.

-Sherman