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
