RFR: 8367901: Calendar.roll(hour, 24) returns wrong result
Naoto Sato
naoto at openjdk.org
Thu Sep 18 15:44:54 UTC 2025
On Wed, 17 Sep 2025 22:37:52 GMT, Justin Lu <jlu at openjdk.org> wrote:
> Please review this PR which addresses an edge case for `GregorianCalendar.roll(int, int)` when the rolled amount would cause the hour to remain the same as before the call. After this change, the expected hour is returned. That is, rolling a full cycle for HOUR (12 hours) and HOUR_OF_DAY (24 hours) should keep the hour the same as before the call.
>
> For example, a calendar with HOUR_OF_DAY == 15,
>
>
> cal.roll(Calendar.HOUR_OF_DAY, 23);
> cal.get(Calendar.HOUR_OF_DAY); // returns 14
>
>
> cal.roll(Calendar.HOUR_OF_DAY, 24);
> // Incorrectly returns 16. A full cycle is expected to return the starting hour (15)
> cal.get(Calendar.HOUR_OF_DAY);
>
>
> cal.roll(Calendar.HOUR_OF_DAY, 25);
> cal.get(Calendar.HOUR_OF_DAY); // returns 16
Looks good. Left some minor suggestions to the test
test/jdk/java/util/Calendar/RollHoursTest.java line 56:
> 54: @FieldSource("hours")
> 55: void HourOfDayTest(int hour) {
> 56: var cal = new GregorianCalendar(2005, 8, 12, PM_HOUR, 30, 45);
IIUC, the regression was caused by the fix to [JDK-8152077](https://bugs.openjdk.org/browse/JDK-8152077), where it jiggles an hour on the transition day. So although I don't think it would fail, but it might be helpful to test on one of those days
test/jdk/java/util/Calendar/RollHoursTest.java line 59:
> 57: // E.g. For hour +50 -> (50 + 15) % 24 = 17
> 58: // For hour -50 -> (24 + (15 - 50) % 24) % 24
> 59: // -> (24 + - 11) % 24 = 13
Not sure "+ -" was intentional, but just wanted to comment. Also this calculation could be a static method, as it is repeated below.
-------------
PR Review: https://git.openjdk.org/jdk/pull/27355#pullrequestreview-3240559003
PR Review Comment: https://git.openjdk.org/jdk/pull/27355#discussion_r2359954295
PR Review Comment: https://git.openjdk.org/jdk/pull/27355#discussion_r2359957874
More information about the core-libs-dev
mailing list