<i18n dev> Fix for small leak in TimeZone_md.c

[Dmitry Samersoff]

Lander,

Changes looks good, few nits below. Please fill free to ignore comments
that is out of scope of your work.


53 I would prefer don't mix #if defined and #ifdef styles.
   Could you please use #if defined(_AIX)

   #if defined(_AIX) need not to be inside #else block, please move
    it below #endif


128 Do you know the platform that fall to #else here?
    I guess we can remove this define.

323 JDK 9 doesn't support Solaris 32bit so this code could be removed.

664 It's better to use memcpy here and copy string with terminating \0
in one line, as you already know the length.

666: It might be easier to use strrchr here (not really important).

678: It's better to use plain strcpy (or ever sprintf) here because you
checked length above.

779: Could you change #ifdef __linux__ to #if defined(__linux__) to keep
one style.

-Dmitry


On 2015-09-03 16:39, Langer, Christoph wrote:
> Here is the updated webrev: http://cr.openjdk.java.net/~simonis/webrevs/2015/8134505/
> 
> -----Original Message-----
> From: Langer, Christoph 
> Sent: Donnerstag, 3. September 2015 12:25
> To: 'Roger Riggs' <Roger.Riggs at Oracle.com>; jdk9-dev at openjdk.java.net
> Cc: 'i18n-dev at openjdk.java.net' <i18n-dev at openjdk.java.net>
> Subject: RE: Fix for small leak in TimeZone_md.c
> 
> Hi Roger,
> 
> thanks for your comments.
> 
> I agree with you and I will remove the platform specifics around line 770.
> 
> As for the suggestions about line 804 and beyond, it would perhaps be nicer to have mutually exclusive sections. However, then I would have to add the common, "not _AIX and not __solaris__", code also to the "__solaris__" part for the case that tz does not equal "localtime", because then the common handling would apply as well. So, to avoid code duplication I'd prefer to leave it as it is.
> 
> Best regards
> Christoph
>  
> 
> -----Original Message-----
> From: jdk9-dev [mailto:jdk9-dev-bounces at openjdk.java.net] On Behalf Of Roger Riggs
> Sent: Dienstag, 1. September 2015 23:46
> To: jdk9-dev at openjdk.java.net
> Subject: Re: Fix for small leak in TimeZone_md.c
> 
> Hi,
> 
> Some comments:
> 
>   - Line 770:  the platform specific conditional compilation can be 
> removed, if *tz is zero, its not a real tz anyway.
> 
>   - Line 804:  " } else"   having the "else" inside the conditional 
> compilation makes the code more fragile.
>     Perhaps just return javatz  and not have to figure out whether those 
> other conditions apply.
> 
>   - It might be clearer if the AIX, Solaris, and linux tail of the code 
> was mutually exclusive.
> 
>   i.e.
> 
> #if defined(_AIX)
>          /* On AIX do the platform to Java mapping. */
>          javatz = mapPlatformToJavaTimezone(java_home_dir, tz);
>          if (freetz != NULL) {
>              free((void *) freetz);
>          }
> #elif defined(__solaris__)
>          /* Solaris might use localtime, so handle it here. */
>          if (strcmp(tz, "localtime") == 0) {
>              javatz = getSolarisDefaultZoneID();
>              if (freetz != NULL) {
>                  free((void *) freetz);
>              }
>          }
> #else  /* otherwise, not _AIX and not __solaris__ */
>          if (freetz == NULL) {
>              /* strdup if we are still working on getenv result. */
>              javatz = strdup(tz);
>          } else if (freetz != tz) {
>              /* strdup and free the old buffer, if we moved the pointer. */
>              javatz = strdup(tz);
>              free((void *) freetz);
>          } else {
>              /* we are good if we already work on a freshly allocated 
> buffer. */
>              javatz = tz;
>          }
> #endif
> 
> $.02, Roger
> 
> On 8/26/2015 5:34 PM, Langer, Christoph wrote:
>> Hi Andrew,
>>
>> today I have posted the following RFR in i18n-dev:
>>
>> when working on TimeZone_md.c lately, I found it worthwhile to do some cleanup on it. Please review my changes.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8134505
>> Webrev: http://cr.openjdk.java.net/~goetz/webrevs/8134505-timeZone/webrev.01/
>>
>> I basically did the following things:
>> - clean up some typos and comment errors
>> - added #include "TimeZone_md.h"
>> - split up the platform specific define sections into an #ifdef <platform1> #elif <platform2> #elif … #endif construct
>> - moved some AIX coding from the #ifdef block at the bottom of the file into the first AIX specific block
>> - AIX function “mapPlatformToJavaTimezone”: use a dynamic malloced buffer instead of a fixed length buffer
>> - refactor function “findJavaTZ_md” to make it more straightforward and to avoid unnecessary mallocs and don’t forget necessary frees
>>
>> I’m also wondering if the “if (tz == NULL || *tz == '\0') {“ of line 770 could be used for all platforms instead of Solaris and AIX only. The other platforms will only do a check if TZ is NULL but not if it is an empty string.
>>
>> I did not follow the suggestion to split up "findJavaTZ_md" in platform versions, though.
>>
>> I'd be grateful for your feedback.
>>
>> Thanks
>> Christoph
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.