Codereview request for 8003680: JSR 310: Date/Time API (original) (raw)

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

Hi Sherman,

Here are my comments on the 310 changes:

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?

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.

Line 214: @SuppressWarnings("fallthrough")?

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

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

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

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

Same as above MinguoDate.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?

Line 131: It should use the default locale for formatting, i.e, Locale.getDefault(Locale.Category.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.

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)

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

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

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

Line 102: "minguoDate" -> "thaiDate"

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

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

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

More information about the core-libs-dev mailing list