[threeten-dev] Codereview request for 8003680: JSR 310: Date/Time API

[Roger Riggs]

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