On Tue, 28 Mar 2023 21:06:34 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 five additional 
> commits since the last revision:
> 
>  - Impl cleanup, add Saturday end day conditional
>  - Rename test, clarify test documentation
>  - Add type to static declaration of Cal.Builder, since only concerned with 
> Gregorian for this test
>  - Pass in amount as 1 directly
>  - Reuse Calendar.Builder

src/java.base/share/classes/java/util/GregorianCalendar.java line 3014:

> 3012:      */
> 3013:     private boolean dayInMinWeek (int day, int startDay, int endDay) {
> 3014:         endDay = endDay == 0

Why `endDay` can be `0`? If the arguments are `Calendar.XXXDAY`, should they be 
between 1 to 7? In other words, why is the above calling site doing 
`getFirstDayOfWeek() - 1`?
I'd add some range checks for the input values.

test/jdk/java/util/Calendar/RollFromLastToFirstWeek.java line 52:

> 50:  */
> 51: public class RollFromLastToFirstWeek {
> 52:     static Calendar.Builder GREGORIAN_BUILDER = new Builder()

Nit: `Calendar.` can be omitted (and can be `private static final`)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1152579044
PR Review Comment: https://git.openjdk.org/jdk/pull/13031#discussion_r1152579338

Reply via email to