<i18n dev> Improve timezone mapping for AIX platform

[Volker Simonis]

Hi Jonathan,

thanks for doing this change. Please find some comments below:

1. please update the copyright year to 2014 in every file you touch

2. in CopyFiles.gmk you can simplify the change by joining the windows
and aix cases because on Windows OPENJDK_TARGET_OS is the same like
OPENJDK_TARGET_OS_API_DIR. So you can write:

ifneq ($(findstring $(OPENJDK_TARGET_OS), windows aix), )

  TZMAPPINGS_SRC := $(JDK_TOPDIR)/src/$(OPENJDK_TARGET_OS)/lib

  $(LIBDIR)/tzmappings: $(TZMAPPINGS_SRC)/tzmappings
    $(call install-file)

  COPY_FILES += $(LIBDIR)/tzmappings

endif

3. regarding the changes proposed by Masayoshi: I agree that we should
put the AIX timezone mapping code in its own function, but I don't
think that getPlatformTimeZoneID() would be the right place. As far as
I understand, getPlatformTimeZoneID() is used to get a platform time
zone id if the environment variable "TZ" is not set. I don't know a
way how this could be done on AIX (@Jonathan: maybe you know?). If
there would be a way to get the time zone from some system files or
so, then we should put this code into the AIX version of
getPlatformTimeZoneID().

But the current AIX code contributed by Jonathan, actually uses the
time zone id from the "TZ" environment variable and just maps it to a
Java compatible time zone id. So I'd suggest to refactor this code
into a function like for example "static const char*
mapPlatformToJavaTimzone(const char* tz)" and call that from
findJavaTZ_md(). I think that would make the code easier to
understand.

@Masayoshi: what do you think, would you agree with such a solution.

4. I agree with Masayoshi that you should use the existing getGMTOffsetID()

5. Please notice that your change breaks all Unix builds except AIX because of:

 759     }
 760 tzerr:
 761     if (javatz == NULL) {
 762         time_t offset;
...
 782     }
 783 #endif

I think that should read:

 759
 760     tzerr:
 761         if (javatz == NULL) {
 762             time_t offset;
...
 782         }
 783 #endif
 784     }

Refactoring the code in an extra function will make that error more
evident anyway.

But please always build at least on one different platform (i.e.
Linux) to prevent such errors in the future.

Regards,
Volker


On Wed, Mar 26, 2014 at 10:27 AM, Masayoshi Okutsu
<masayoshi.okutsu at oracle.com> wrote:
> Hi Jonathan,
>
> The AIX specific code looks OK to me. But I'd suggest the code be moved to
> getPlatformTimeZoneID() for AIX, which just returns NULL currently. Also
> there's a function for producing a fallback ID in "GMT±hh:mm",
> getGMTOffsetID() which can be called in tzerr.
>
> Thanks,
> Masayoshi
>
>
> On 3/26/2014 3:47 PM, Jonathan Lu wrote:
>>
>> Hi ppc-aix-port-dev, core-libs-dev,
>>
>> Here's a patch for JDK-8034220,
>>
>> http://cr.openjdk.java.net/~luchsh/JDK-8034220/
>>
>> It is trying to add the a more complete timezone mapping mechanism for AIX
>> platform.
>> the changes are primarily based on IBM's commercial JDK code, which
>> includes:
>>
>> - A new timezone mapping file added under directory jdk/src/aix/lib/
>> - Code to parse above config file, changed file is
>> src/solaris/native/java/util/TimeZone_md.c
>> - And also makefile change in make/CopyFiles.gmk to copy the config file
>>
>> Could you pls help to review and give comments ?
>>
>> Cheers
>> - Jonathan
>
>