Hi Christoph, the change looks good. Thanks for doing this clean-up.
I'll sponsor the change. Regards, Volker On Wed, Sep 9, 2015 at 3:08 PM, Langer, Christoph <christoph.lan...@sap.com> wrote: > Hi Dmitry, > > let me comment on your suggestions: > 663 it might be better to use str*r*chr > -> a TZ variable could look like > TZ="MET-1METDST,M3.5.0/02:00:00,M10.5.0/03:00:00". So there could be multiple > instances of the character ',' and I'm only interested in the first part of > TZ, e.g. in "MET-1METDST" in this example case. So why should I want to use > strrchr? > > 666 memcpy(tz_buf, tz, tz_len+1); > 667 is not necessary > -> well, in the case of a TZ variable like above, tz_len would be 11. If I > now copy 12 characters, I don't copy a trailing 0 but the ',' character that > strchr found. So I have to memcpy exactly tz_len characters and set the > trailing 0 myself. > > As for the P4 CR - I can do it, when I'm editor eventually :-) > > Can I consider this change as reviewed then? > > Thanks > Christoph > > -----Original Message----- > From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com] > Sent: Mittwoch, 9. September 2015 09:12 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: jdk9-...@openjdk.java.net; i18n-dev@openjdk.java.net; Roger Riggs > <roger.ri...@oracle.com> > Subject: Re: Fix for small leak in TimeZone_md.c > > Christoph, > > Looks good for me. > > 663 it might be better to use str*r*chr > 666 memcpy(tz_buf, tz, tz_len+1); > 667 is not necessary > >> - The part following line 323 for Solaris32 can probably be removed >> but I don't want to be the guy that does this > > OK. Could you file a P4 CR to get it removed? > > -Dmitry > > On 2015-09-08 17:06, Langer, Christoph wrote: >> Hi Dmitry, >> >> thanks for your review. >> >> - I followed your suggestions to replace #ifdef with #if defined() at all >> places >> - I changed the usage of readdir_r to readdir64_r, as it is done in >> UnixFileSystem_md.c as well. So we can remove the define from line 129 in >> the original TimeZone_md.c >> - The part following line 323 for Solaris32 can probably be removed but I >> don't want to be the guy that does this >> - 664 and 666 - I changed the construct a bit and I'm using strchr and >> memcpy now :-) >> - 678: changed to use strcpy as suggested >> >> Here is the new webrev: >> http://cr.openjdk.java.net/~simonis/webrevs/2015/8134505.v1/ >> >> Best regards, >> Christoph >> >> -----Original Message----- >> From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com] >> Sent: Samstag, 5. September 2015 19:47 >> To: Langer, Christoph <christoph.lan...@sap.com>; Roger Riggs >> <roger.ri...@oracle.com> >> Cc: jdk9-...@openjdk.java.net; i18n-dev@openjdk.java.net >> Subject: Re: Fix for small leak in TimeZone_md.c >> >> 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.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 >>> >> >> > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
