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

Naoto Sato naoto.sato at
Thu Jan 17 23:55:46 UTC 2013

Hi Sherman,

Here are my comments on the 310 changes:

- java/util/

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/

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 

- java/time/

Line 214: @SuppressWarnings("fallthrough")?

- java/time/calendar/

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

- java/time/calendar/

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

- java/time/calendar/

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

- java/time/calendar/

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

- java/time/calendar/

Same as above

- java/time/format/

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 

- java/time/format/

Line 131: It should use the default locale for formatting, i.e, 

- java/time/format/

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/

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

- java/time/format/

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/

Line 102: "minguoDate" -> "thaiDate"

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

- java/time/zone/

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


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
> and the latest javadoc
> Review comments can be sent to the threeten-dev email list [2] and/or
> core-libs-dev email list[3].
> Thanks,
> Sherman
> [1]
> [2] threeten-dev @
> [3] core-libs-dev @

More information about the core-libs-dev mailing list