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
