On Thu, 12 Nov 2020 15:41:56 GMT, Stephen Colebourne <scolebou...@openjdk.org>
wrote:
>> 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.
Fixed.
> 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;
Fixed.
> 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);
> }
Fixed.
> 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).
Fixed.
> 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"
Fixed.
-------------
PR: https://git.openjdk.java.net/jdk/pull/938
