<i18n dev> [14] RFR: 8232871: Host Locale Provider on Mac does not return translated values of Japanese calendar

[naoto.sato at oracle.com]

Hi Brent,

Thank you for the review! Please see my comments below.

On 11/6/19 3:27 PM, Brent Christian wrote:
> Hi, Naoto
> 
> Looks pretty good.  I have a few comments:
> 
> -- 
> src/java.base/macosx/classes/sun/util/locale/provider/HostLocaleProviderAdapterImpl.java 
> 
> 
>   572                     map = new HashMap<>();
> 
> FWIW, I believe the HashMap could be pre-sized using 'names.length'.

Modified to specify the initial capacity.

> 
> 
> -- 
> src/java.base/macosx/native/libjava/HostLocaleProviderAdapter_md.c
> 
>   457     jobjectArray ret = NULL;
> 
> Is 'ret' needed?

Removed.

> 
> 
>   713                 int sindex = cal == kCFJapaneseCalendar ? 
> JAPANESE_MEIJI_INDEX : 0;
> 
> So here we are moving 'sindex' past earlier eras in 'cferas', in order 
> to start at Meiji, yes?

Yes. While JDK's Japanese calendar provides eras since Meiji and 
"BeforeMeiji", which is a hypothetical era for all prior eras, macOS's 
Japanese calendar returns all Japanese eras. Copying should be done only 
for Meiji and after.

> 
> 
>   740                 if (wdays != NULL) {
>   741                     array_length = (*env)->GetArrayLength(env, 
> wdays);
>   742                 } else {
>   743                     array_length = dayCount + 1;
> 
> It doesn't look like 'array_length' is needed for the 'wdays != NULL' 
> case.  In fact, could 740-743 be changed to:
> 
> if (wdays == NULL) {
>      jsize array_length = dayCount + 1;
> 
> ?

You're absolutely right. These are copied from Windows' counterpart and 
left over.

Here is the updated webrev:

https://cr.openjdk.java.net/~naoto/8232871/webrev.01/

Naoto

> 
> Thanks,
> -Brent
> 
> On 11/5/19 12:48 PM, naoto.sato at oracle.com wrote:
>> Hello,
>>
>> Please review the fix to the following issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8232871
>>
>> The proposed changeset is located at:
>>
>> https://cr.openjdk.java.net/~naoto/8232871/webrev.00/
>>
>> Implementation for getting calendar display names was missing in 
>> macOS's host adapter.
>>
>> Naoto