zlib1.2.3

[Martin Buchholz]

On Sun, Aug 23, 2009 at 21:56, Xueming Shen <Xueming.Shen at sun.com> wrote:

> Martin Buchholz wrote:
>
>> On Sat, Aug 22, 2009 at 20:37, Xueming Shen<Xueming.Shen at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20090824/8a6536d4/attachment.html>