On Fri, 24 Mar 2023 18:48:41 GMT, Justin Lu <j...@openjdk.org> wrote:
>> This PR fixes the bug which occurred when `Calendar.roll(WEEK_OF_YEAR)` >> rolled into a minimal first week with an invalid `WEEK_OF_YEAR` and >> `DAY_OF_WEEK` combo. >> >> For example, Rolling _Monday, 30 December 2019_ by 1 week produced _Monday, >> 31 December 2018_, which is incorrect. This is because `WEEK_OF_YEAR` is >> rolled from 52 to 1, and the original `DAY_OF_WEEK` is 1. However, there is >> no Monday in week 1 of 2019. This is exposed when a future method calls >> `Calendar.complete()`, which eventually calculates a `fixedDate` with the >> invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo. >> >> To prevent this, a check is added for rolls into week 1, which determines if >> the first week is minimal. If it is indeed minimal, then it is checked if >> `DAY_OF_WEEK` exists in that week, if not, `WEEK_OF_YEAR` must be >> incremented by one. >> >> After the fix, Rolling _Monday, 30 December 2019_ by 1 week produces >> _Monday, 7 January 2019_ > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Remove max week methods accidentally comitted Looks good. Mostly cosmetic comments follow src/java.base/share/classes/java/util/GregorianCalendar.java line 1315: > 1313: // rolling up into week 1, as the existing checks > 1314: // sufficiently handle rolling down into week 1. > 1315: if (newWeekOfYear == 1 && (isInvalidWeek1())) { Extra parens around `isInvalidWeek1()` src/java.base/share/classes/java/util/GregorianCalendar.java line 1316: > 1314: // sufficiently handle rolling down into week 1. > 1315: if (newWeekOfYear == 1 && (isInvalidWeek1())) { > 1316: if (amount > 0) { Can be folded into the above `if` src/java.base/share/classes/java/util/GregorianCalendar.java line 3015: > 3013: private boolean isMinWeek (int days) { > 3014: return days >= getMinimalDaysInFirstWeek(); > 3015: } Since this method is only used above once, maybe make the check inline? src/java.base/share/classes/java/util/GregorianCalendar.java line 3032: > 3030: // not between 6 7 1 2 3 > 3031: return !(day >= startDay || day <= endDay); > 3032: } Could be more readable if the method does not have `Not`, i.e, `daysInMinWeek`? This would remove the negation in the return statements. test/jdk/java/util/Calendar/RollToMinWeek.java line 28: > 26: * @bug 8225641 > 27: * @summary Test the behavior of Calendar.roll(WEEK_OF_YEAR) when the > last week > 28: * is rolled up into a minimal week 1 of the same year `minimal week 1` may be unclear. test/jdk/java/util/Calendar/RollToMinWeek.java line 43: > 41: /** > 42: * Test to validate the behavior of Calendar.roll(WEEK_OF_YEAR, +1) > 43: * when rolling into a minimal week 1 from the max week. WEEK_OF_YEAR can IIUC, `minimal` and `max` mean `first` and `last` respectively? I'd rather change those as it would confuse with `minimal days in the first week`. test/jdk/java/util/Calendar/RollToMinWeek.java line 52: > 50: public void rollUpTest(Calendar calendar, String[] validDates){ > 51: if (calendar instanceof GregorianCalendar) { > 52: testRoll(calendar, validDates, +1); If the amount is fixed to `+1`, no need to pass it as an argument test/jdk/java/util/Calendar/RollToMinWeek.java line 98: > 96: for (int firstDay = 1; firstDay <= 7; firstDay++) { > 97: calList.add(Arguments.of(buildCalendar("gregory", > firstDay, weekLength, > 98: dayOfMonth, 11, 2019), > validDates[date])); `11` may better be replaced with `Calendar.DECEMBER`. test/jdk/java/util/Calendar/RollToMinWeek.java line 114: > 112: calBuilder.setDate(year, month, dayOfMonth); > 113: return calBuilder.build(); > 114: } `Builder` instance can be reused. Also since Builder can method chain, you could write static Calendar.Builder CAL_BUILDER = new Builder().setCalendarType(type); return CAL_BUILDER .setWeekDefinition(...) .setDate(...) .build(); ------------- PR Review: https://git.openjdk.org/jdk/pull/13031#pullrequestreview-1359843143 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149772051 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149771259 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149777645 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149779427 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149838007 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149838132 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149843461 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149847360 PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1149851739