Codereview request for 8003680: JSR 310: Date/Time API

[Naoto Sato]

Hi Sherman,

Here are my comments on the 310 changes:


- java/util/Formatter.java

Line 4125: To do a locale neutral case mapping, use Locale.ROOT instead 
of Locale.US.

Line 4161-4169: Remove this as it is commented out. Also "import 
sun.util.locale.provider.TimeZoneNameUtility" is not needed either.

Line 4173, 4183, 4195: The code assumes Locale.US if "l == null." Is 
this correct? Should it be Locale.ROOT?


- java/time/DayOfWeek.java

Line 298-300: Maybe just me, but I thought the convention for "throws" 
was to consolidate conditions into a single "throws" line for the same 
exception.


- java/time/ZoneOffset.java

Line 214: @SuppressWarnings("fallthrough")?


- java/time/calendar/ChronoDateImpl.java:

In the example in the class description, "%n" should be "\n" for the new 
lines in the printf()s.


- java/time/calendar/HijrahDeviationReader.java

Line 224: Is it OK to catch all Exception here, and pretends as if 
nothing happened?


- java/time/calendar/JapaneseChrono.java

Line 166-172: How come it includes "Keio"? Isn't the era before "Meiji" 
"Seireki"?


- java/time/calendar/MinguoDate.java

Is it OK to NOT serialize "isoDate"? JapaneseDate.java has @serial on 
this field.

- java/time/calendar/ThaiBuddhistDate.java

Same as above MinguoDate.java

- java/time/format/DateTimeBuilder.java

There are several "TODO"s in here.

Line 357: It is commented that return value is for ZoneOffset/ZoneId. 
But since it is public, could this be safe? Can arbitrary app modify it 
maliciously?


- java/time/format/DateTimeFormatSymbols.java

Line 131: It should use the default locale for formatting, i.e, 
Locale.getDefault(Locale.Category.FORMAT)


- java/time/format/DateTimeFormatter.java

Line 411: DateTimeException needs to be described, as it is thrown in 
this method.

Line 486: The DateTimeException needs to be on @throws clause.


- java/time/format/DateTimeFormatterBuilder

Line 174: The type for "padNextChar" should be "int" to accommodate 
supplementary characters. Not only this particular instance, it looks 
like there are bunch of places that use ++/-- for character iteration. 
Are they OK?

Line 236: "sensitive" -> "insensitive"

Line 276: "strict" -> "lenient"

Line 1440: use Locale.getDefault(Locale.Category.FORMAT)


- java/time/format/DateTimeFormatters.java

Line 271, 294, 317, 340: Locale.getDefault() -> 
Locale.getDefault(Locale.Category), "default locale" -> "default FORMAT 
locale"


- java/time/format/DateTimeParseContext.java

Line 194, 195: Should those to[Lower/Upper]Case() take Locale.ROOT as an 
argument? Remember the Turkish 'i' case?


- java/time/overview.html

Should the contents here be moved to package.html? Should we continue to 
use "Threeten" name?


- java/time/temporal/Chrono.java

Line 102: "minguoDate" -> "thaiDate"

Line 212: The comment is kind of cryptic. Looks like not "removing" but 
"registering"


- java/time/zone/TzdbZoneRulesProvider.java

Line 171: Is it OK to catch all exceptions here and ignore?


- sun/tools/tzdb/*

Should the compile be moved under "make" directory, instead of "src"?


Naoto

On 1/15/13 4:13 PM, 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].
>
> Thanks,
> Sherman
>
> [1] http://openjdk.java.net/projects/threeten
> [2] threeten-dev @ openjdk.java.net
> [3] core-libs-dev @ openjdk.java.net
>
>