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

[Xueming Shen]

Thanks Naoto!

We will see how many we can update tomorrow.

Regarding the Locale.ROOT vs Locale.US, it appears we are now using
Locale.US in j.u.Formatter for l==null case in existing. (Formatter is
earlier than the the 1.6 ROOT). So I may keep the US for now for the
consistency. It may be a good small Locale.US cleanup project later.

-Sherman

On 01/17/2013 03: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.
>
>
> - 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
>>
>>
>