On Fri, Aug 21, 2009 at 11:09, Xueming Shen <xueming.s...@sun.com> wrote: > > Martin, webrev has been updated to > > (1)use unidiff (and the correct order of src and target) > (2)put those diffs into a patches dir > (3)remove the minigzip.c from the ws (this one is not included in the file_c > list...)
Thanks. ------------- 35 +/* for _LP64 */ 36 +#include <sys/types.h> 37 + 38 /* 39 * If you *really* need a unique prefix for all types and library functions, 40 * compile with -DZ_PREFIX. The "standard" zlib should be compiled without it. 41 @@ -261,7 +288,11 @@ 42 typedef unsigned char Byte; /* 8 bits */ 43 #endif 44 typedef unsigned int uInt; /* 16 bits or more */ 45 +#ifdef _LP64 46 +typedef unsigned int uLong; /* 32 bits or more */ 47 +#else 48 typedef unsigned long uLong; /* 32 bits or more */ 49 +#endif zlib (intentionally) doesn't have much of a configure layer, so we take our best shots at portability. I think the above probably works fine, but I would do the very simple typedef unsigned int uLong; since unsigned int is likely to be 32 bits on any platform the JDK is ever likely to run on. Only early Dec Alpha systems made the mistake of having a 64-bit int. The 32-bit vs. 64-bit types are a mess. I notice that crc32 tries to define its own "u4" portability type ------------- 31 @@ -39,7 +63,7 @@ 32 typedef unsigned int u4; 33 # else 34 # if (ULONG_MAX == 0xffffffffUL) 35 - typedef unsigned long u4; 36 + typedef uLong u4; 37 # else 38 # if (USHRT_MAX == 0xffffffffUL) 39 typedef unsigned short u4; Using uLong in the above is probably not right, since comparison against ULONG_MAX means the corresponding type is unsigned long (not that it matters) I'm suspicious that the changes in crc32.c might not be needed, because nobody else is having to make these changes. Perhaps use of "unsigned long" in crc32.c is correct because in these cases either a 32-bit or 64-bit type will do? Did you actually encounter functional problems with the original code? ------------- Meta-comment: It would be nice if Sun engineers were able to keep changes explicitly compatible with the zlib license (i.e. no GPL), so that they were upstreamable to the zlib team. ------------- Martin > > > http://cr.openjdk.java.net/~sherman/zlib123/webrev > > Thanks > Sherman > > Martin Buchholz wrote: >> >> I like very much that you are saving local changes in the form of patches >> (I've been advocating this style of modification of upstream sources >> for a while now), but... >> - please generate your patches in "unidiff" form (diff -u) >> - the extension ".patch" is, I think, preferable to ".diff" >> (or put them into a "patches" directory, as debian does) >> >> I don't think we need minigzip.c - it's a kind of "demo" program. >> >> Do we really need to rename files? That's quite confusing. >> >> The pervasive change of >> uLong => unsigned long >> seems unnecessary - just get the typedef of uLong correct. >> >> Martin >> >> On Thu, Aug 20, 2009 at 19:49, Xueming Shen <xueming.s...@sun.com >> <mailto:xueming.s...@sun.com>> wrote: >> >> Here is the latest webrev for updating the zlib from the aged >> 1.1.3 to the latest version 1.2.3 >> >> >> http://cr.openjdk.java.net/~sherman/zlib123/webrev >> <http://cr.openjdk.java.net/%7Esherman/zlib123/webrev> >> >> Alan, Kumar Martin, thanks for the comments, while I'm continuing >> running more tests:-) here are the >> reply to your comments/questions. >> >> (1) The ZLIB_VERSION has been moved into common/Defs.gmk. >> (2) The copyright you quoted in the original copyright of >> zlib1.1.3 (it was there is jdk6 and earlier before we opensourced >> the jdk7). >> (3) Yes, I have run the regression tests in tools/pack200 and >> those in launcher, as well as other jar, zip related. >> (4) I just jprt-ed a full forest/control build including >> everything, it finished without any problem >> (5) So far the "large zip support" still holds on all platforms I >> tested on (Solaris sparc 32-bit/x86 32-bit, Linux x64 and >> Windows32-bit. >> (6)I would still prefer to get the source code from zlib.org >> <http://zlib.org>. I took a quick scan at the ubuntu diff, don't >> see the value of replacing the one >> we have. >> (7) The java port change log has been moved to >> >> http://cr.openjdk.java.net/~sherman/zlib123/webrev/src/share/native/java/util/zip/zlib-1.2.3/ChangeLog_java.html >> >> <http://cr.openjdk.java.net/%7Esherman/zlib123/webrev/src/share/native/java/util/zip/zlib-1.2.3/ChangeLog_java.html> >> >> Sherman >> >> >> >> >> >> >> >
