Ok, looks fine to me.
Thanks, Roger
On 9/3/2015 9:39 AM, 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.ri...@oracle.com>; jdk9-...@openjdk.java.net
Cc: 'i18n-dev@openjdk.java.net' <i18n-dev@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-boun...@openjdk.java.net] On Behalf Of Roger
Riggs
Sent: Dienstag, 1. September 2015 23:46
To: jdk9-...@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
