On Sun, Aug 23, 2009 at 21:56, Xueming Shen <xueming.s...@sun.com> wrote:
> Martin Buchholz wrote: > >> On Sat, Aug 22, 2009 at 20:37, Xueming Shen<xueming.s...@sun.com> wrote: >> >> >>> ------------- >>>> 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) >>>> >>>> >>>> >>> >> # if (UINT_MAX == 0xffffffffUL) >> typedef unsigned int u4; >> # else >> # if (ULONG_MAX == 0xffffffffUL) >> typedef unsigned long u4; >> # else >> # if (USHRT_MAX == 0xffffffffUL) >> typedef unsigned short u4; >> >> I believe that on all platforms where the JDK will be built, >> the first test UINT_MAX == 0xffffffffUL will be true, >> so the suggested change will never pass the preprocessor. >> I believe it to both be wrong and to have no effect, and increase the >> size of local changes - but still OK to commit if you insist! >> >> > Oh, I see your point. Are you saying this "local change" is not necessary > since it never gets > preprocessed? No, more than that - the code is defining a derived type u4 from primitive types - it should not be using other derived types. BTW, u4 is of size *exactly* 4 bytes, while uLong I think is of size *at least* 4 bytes, so these are slightly semantically different. Looking again at zlib's types, I see that uLong is unconditionally typedef'ed to "unsigned long" so in an unmodified distribution they can be used interchangeably (although a little unclean - why have a typedef in the first place?) zlib definitely claims to support 64-bit platforms - see FAQ. It should Just Work to let uLong be always unsigned long, even though that might be "too large" for the data being provided on LP64 systems. ---- You should probably update zconf.h to include unistd.h as if you had run configure, at least on Unix. 318 #if 0 /* HAVE_UNISTD_H -- this line is updated by ./configure */ 319 # include <sys/types.h> /* for off_t */ 320 # include <unistd.h> /* for SEEK_* and off_t */ It looks like you won't get large file support on 32-bit systems without struggling with configury stuff. Or maybe we never use zlib in the JDK to directly access files via seek and off_t? OK, I admit I did the replace all in emacs:-) Maybe I should have touched > those that affected. But given the nature of crc32, I'm pretty sure a > 32-bit unsigned integer > is what it should be (and is intended,) as the code purposely defines the > "u4". > > An alternative is to keep the crc32.c/h untouched, change the crc32() > declare in zlib.h to > unsigned long and then change the rest of jdk (our CRC.c and the pack code > to prepare > a 64-bit unsigned long on 64-bits), is this the direction you would prefer > to? > I don't see why the rest of the jdk has to be changed, or why a completely unmodified zlib cannot be used (although some modifications, like for total_in are well motivated) If we are ever to support using alternative zlibs like the system zlib, as icedtea has done, then we cannot depend on enhancements like a 64-bit total_in. Perhaps even that change should not be made. Martin > > Sherman >
