Hi Naoto,
Thank you for the review, comments inline.
Some issues are still to be addressed.
On 1/17/2013 6:55 PM, Naoto Sato wrote:
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.
This style is used by JSR 310 and will need to be reconciled with the JDK
before release.
- java/time/ZoneOffset.java
Line 214: @SuppressWarnings("fallthrough")?
intentional; fixed with @SuppressWarnings
- java/time/calendar/ChronoDateImpl.java:
In the example in the class description, "%n" should be "\n" for the
new lines in the printf()s.
According to java.util.Format %n is the platform specific line separator.
"\n" would, I expect, work just as well.
- java/time/calendar/HijrahDeviationReader.java
Line 224: Is it OK to catch all Exception here, and pretends as if
nothing happened?
That check is an optimization of the library path. Even if it fails, the
file will be checked with the unoptimized library path.
What is the convention for reporting configuration errors? It might be
applicable in this case.
- java/time/calendar/JapaneseChrono.java
Line 166-172: How come it includes "Keio"? Isn't the era before
"Meiji" "Seireki"?
Those resources are unused and should be removed.
The Era names are provided by the JapaneseEra class.
- java/time/calendar/MinguoDate.java
Is it OK to NOT serialize "isoDate"? JapaneseDate.java has @serial on
this field. \
The @serial and readObject in JapaneseDate is out of date, the
serialized form for
all of ChronoLocalDates use writeReplace to put an instance of "Ser" in
the stream.
- java/time/calendar/ThaiBuddhistDate.java
Same as above MinguoDate.java
Same as MinguoDate
- java/time/format/DateTimeBuilder.java
There are several "TODO"s in here.
DateTimeBuilder is in the process of being redesigned and will be
updated for M7.
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?
DateTimeBuilder is a working context for resolving individual fields
during parsing.
Its instances are short lived and not shared.
- 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.
added
Line 486: The DateTimeException needs to be on @throws clause.
added
- 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?
The overview.html is present only for the JSR 310 Public Review as
an overview of the API and will not be present in the JDK.
Most of its contents have been integrated into java.time package javadoc.
- java/time/temporal/Chrono.java
Line 102: "minguoDate" -> "thaiDate"
fixed
Line 212: The comment is kind of cryptic. Looks like not "removing"
but "registering"
Corrected.
- java/time/zone/TzdbZoneRulesProvider.java
Line 171: Is it OK to catch all exceptions here and ignore?
Similar to the treatment in HijrahDeviationReader; the library path
optimization
may fail but the file checks are done later.
- 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
