On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added a test case for user defined temporal field resolution with day > period. src/java.base/share/classes/java/time/format/Parsed.java line 550: > 548: long midpoint = dayPeriod.mid(); > 549: fieldValues.put(HOUR_OF_DAY, midpoint / 60); > 550: fieldValues.put(MINUTE_OF_HOUR, midpoint % 60); Set `dayPeriod = null` here, as it has been successfully used. src/java.base/share/classes/java/time/format/Parsed.java line 502: > 500: HOUR_OF_AMPM.checkValidValue(hoap); > 501: } > 502: if (dayPeriod.includes((hoap % 24 + 12) * 60)) { Pretty sure the 24 should be 12. Also, it should use `Math.floorMod()` to handle `HOUR_OF_AMPM = -1` (which is 11 o'clock). Also, this doesn't take into account minutes. So if the day period rule is afternoon1 to 18:30 and evening1 after 18:30, and the input is `HOUR_OF_AMPM = 6, MINUTE_OF_HOUR = 45`, this will fail to resolve, Something like this should work (no need for `updateCheckDayPeriodConflict`): long hoap = fieldValues.remove(HOUR_OF_AMPM); if (resolverStyle == ResolverStyle.LENIENT) { HOUR_OF_AMPM.checkValidValue(hoap); } Long mohObj = fieldValues.get(MINUTE_OF_HOUR); long moh = mohObj != null ? Math.floorMod(mohObj, 60) : 0; long excessHours = dayPeriod.includes(Math.floorMod(hoap, 12) + 12 * 60 + moh ? 12 : 0; long hod = Math.addExact(hoap, excessHours); updateCheckConflict(HOUR_OF_AMPM, HOUR_OF_DAY, hod); dayPeriod = null; src/java.base/share/classes/java/time/format/Parsed.java line 546: > 544: > 545: // Set the hour-of-day, if not exist and not in STRICT, to > the mid point of the day period or am/pm. > 546: if (!fieldValues.containsKey(HOUR_OF_DAY) && resolverStyle > != ResolverStyle.STRICT) { Since this logic replaces both `HOUR_OF_DAY` and `MINUTE_OF_HOUR` I think it should only be invoked if both do not exist. Beyond that, it doesn't really make sense to do this logic if second/sub-second are present. Thus, it needs to check all four fields and can then directly set the time. if (!fieldValues.containsKey(HOUR_OF_DAY) && !fieldValues.containsKey(MINUTE_OF_HOUR) && !fieldValues.containsKey(SECOND_OF_MINUTE) && !fieldValues.containsKey(NANO_OF_SECOND) && resolverStyle != ResolverStyle.STRICT) { if (dayPeriod != null) { long midpoint = dayPeriod.mid(); resolveTime(midpoint / 60, midpoint % 60, 0, 0); dayPeriod = null; } else if (fieldValues.containsKey(AMPM_OF_DAY)) { long ap = fieldValues.remove(AMPM_OF_DAY); if (resolverStyle == ResolverStyle.LENIENT) { resolveTime(Math.addExact(Math.multiplyExact(ap, 12), 6), 0, 0, 0); } else { // SMART AMPM_OF_DAY.checkValidValue(ap); resolveTime(ap * 12 + 6, 0, 0, 0); } src/java.base/share/classes/java/time/format/Parsed.java line 568: > 566: if (dayPeriod != null) { > 567: // Check whether the hod is within the day period > 568: updateCheckDayPeriodConflict(HOUR_OF_DAY, hod); With the other changes, this is the only use of this method and it can therefore be simplified (no need to check for conflicts, just whether it matches the day period). test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 778: > 776: {"h B", ResolverStyle.SMART, "59 in the morning", 11}, > 777: > 778: {"H B", ResolverStyle.LENIENT, "47 at night", 23}, This test should be split in two - smart (fails) and lenient (succeeds). The lenient tests should ensure that `p.query(DateTimeFormatter.parsedExcessDays())` returns the expected number of excess days. I'd also add a test for a negative value such as "-2 at night" ------------- PR: https://git.openjdk.java.net/jdk/pull/938
