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

Roger Riggs Roger.Riggs at oracle.com
Fri Jan 18 18:36:01 UTC 2013

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

More information about the core-libs-dev mailing list