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.s...@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
