Hi Roger, Sorry for the missing the bug id in the subject, added it. Better late than never :-) On 2/20/19 6:41 PM, Roger Riggs wrote:
Hi Naoto,
The fix looks fine.
The direction for new tests is to give them functional names, not bugids. Is there a suitable name?
Renamed the test case, and modified @summary text accordingly.
CalendarDataUtility.java: 260; the assert is just documentation right? Its rare to see asserts enabled except in test contexts.
Yes, it's meant for jtreg environment so that they are logged (if ever). Updated webrev: http://cr.openjdk.java.net/~naoto/8218960/webrev.01/ Naoto
Thanks, Roger
On 2/20/19 5:54 PM, naoto.sato@oracle.com wrote:
Thanks, Nishit.
I'd still like to ask for a review from a Reviewer.
Naoto
On 2/20/19 12:33 AM, Nishit Jain wrote:
Hi Naoto,
Thanks for the explanation. Change looks fine to me.
Regards, Nishit Jain On 19-02-2019 22:51, Naoto Sato wrote:
Hi Nishit,
The reason is that "US" is the only required locale in the JDK (cf. Locale.getAvailableLocales(). In fact, initially I supplied "001" with it, as it means the "world" in CLDR, but it broke some existing tests. "001" returns "MONDAY" as the first day of week, whereas "SUNDAY" in US. For the compatibility reason, I had to resort to "US". I am not sure we want to hardcode "1" in this case without any convincing reason.
Naoto
On 2/19/19 6:37 AM, Nishit Jain wrote:
Hi Naoto,
Why is the default region set to "US" if there is no region specified in the locale? is this the default behavior of "first day of week" and "minimal days in first week" when a region is missing or the default behavior is that it returns "1"? Can't we just return "1" instead of setting the region to "US"?
Regards, Nishit Jain On 16-02-2019 04:25, Naoto Sato wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8218960
The proposed changeset is located at:
http://cr.openjdk.java.net/~naoto/8218960/webrev.00/
The CONFIG message was generated because CLDRCalendarDataProviderImpl was returning null for locales without region. Use "US" as the default region in such a case.
Naoto