On Sun, 27 Sep 2026 11:25:42 GMT, sbracely <[email protected]> wrote:
>> withVariant() used getDayOfYear() as the month-length bound when clamping
>> the day-of-month to the target variant.
>>
>> Use getMonthLength(), matching resolvePreviousValid.
>>
>> getMonthLength() now calls checkCalendarInit().
>>
>> HijrahConfigTest copies a custom variant whose year 1300 month 1 has 29 days
>> and checks that day 30 clamps to 29.
>>
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> sbracely has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update the test configuration properties
I think the change looks fine and using `getMonthLength` instead of
`getDayOfYear` does indeed look like the right choice here for the clamping
logic.
Just curious if this issue was discovered in an application or through some
type of deliberate testing. Using a custom Hijrah variant is quite a special
use case.
test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigCheck.java line 79:
> 77:
> 78: // Variant configuration test
> 79: HijrahChronology variantChronology = (HijrahChronology)
> Chronology.of("islamic-variant");
I would use the variable for "islamic-variant" that was defined above.
test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigCheck.java line 82:
> 80: HijrahDate hijrahDateWithVariant = HijrahDate.of(1300, 1,
> 30).withVariant(variantChronology);
> 81: HijrahDate expected = variantChronology.date(1300,1,29);
> 82: if (!hijrahDateWithVariant.equals(expected)) {
I would also add a brief comment describing that the variant only supports 29
days so it gets clamped, that way a future reader does not have to dig into the
properties file to figure it out.
test/jdk/java/time/nonjunit/java/time/chrono/HijrahConfigTest.java line 35:
> 33: * @test
> 34: * @summary Tests whether a custom Hijrah configuration properties file
> works correctly
> 35: * @bug 8187987 8392848
Bug header needs to be updated with the JBS issue.
-------------
PR Review: https://git.openjdk.org/jdk/pull/33079#pullrequestreview-5357962567
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4137942301
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4137964370
PR Review Comment: https://git.openjdk.org/jdk/pull/33079#discussion_r4137967876
