On Thu, 10 Dec 2020 21:12:29 GMT, Naoto Sato <na...@openjdk.org> wrote:
> Hello,
>
> Please review the changes to the subject issue. getMinimalDaysInFirstWeek()
> for Windows has been implemented to suffice the bug claim.
Looks good to me. Some minor comments below.
src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
line 78:
> 76: // CalendarData value types
> 77: private static final int CD_FIRSTDAYOFWEEK = 0;
> 78: private static final int CD_MINIMALDAYSINFIRSTWEEK = 1;
Do we want to keep the naming consistent, doing the same change to, e.g. the
corresponding macosx impl?
src/java.base/windows/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java
line 373:
> 371: CD_FIRSTWEEKOFYEAR);
> 372: return switch (firstWeek) {
> 373: case 1 -> 7;
Would it be good to use constants or enum instead of literal, or maybe at least
a note for the case numbers.
test/jdk/java/util/Locale/LocaleProvidersRun.java line 177:
> 175:
> 176: //testing 8257964 fix. (macOS/Windows only)
> 177: testRun("HOST", "bug8257964Test", "", "", "");
This test runs only if the platform locale is en-GB. Do we know if the test
system run tests on multiple locales? From the console output unfortunately,
it's impossible to tell which specific tests were run
-------------
Marked as reviewed by joehw (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1741
