Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #39 #42

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions zip/zlib.nim
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,24 @@ else:
const libz = "libz.so.1"

type
Uint* = int32
Ulong* = uint32
Ulongf* = uint32
Uint* {.importc: "uInt", header: "<zlib.h>".} = cuint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing a type alias should not be used, it's a mess in Nim's C codegen already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't see an alternative to it. The type is different on different platforms. It is not just that different platforms are different. Different versions changed the type behind the type alias as well. What I don't like about it is, sizeof(Uint) will be the same as cuint, so using Uint can't be seen as safe. In combination of Uint being exported, I see this as a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Araq I am reviewing my old PR. This PR is necessary as it is in order to fix #39 properly. If you say importing a type alias is a mess, then go fix it. It is not my fault that it is a mess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not my fault you can only see this one particular way to fix the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a different solution that doesn't break compatibility with existing versions of zlib, go ahead and implement the fix. Then when you realized that the import of the type alias is actually the correct way to handle this, you can still come back merge this and clean up the parts in the C codegen that you don't like.

If you however don't plan to fix bugs in zlib anymore and you don't want to be bothered with other peoples pull request because you are too busy implementing new feauturs in Nim, then you can still archive this repository to indicate that it isn't maintained anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I can also simply close this PR as it's untested on Windows, ignores the fact that the existing wrapper doesn't depend on a header file, etc etc...

Ulong* {.importc: "uLong", header: "<zlib.h>".} = uint
Ulongf* {.importc: "uLongf", header: "<zlib.h>".} = uint
Pulongf* = ptr Ulongf
ZOffT* = int32
ZOffT* {.importc: "z_off_t", header: "<zlib.h>".} = int
Pbyte* = cstring
Pbytef* = cstring
Allocfunc* = proc (p: pointer, items: Uint, size: Uint): pointer{.cdecl.}
FreeFunc* = proc (p: pointer, address: pointer){.cdecl.}
InternalState*{.final, pure.} = object
PInternalState* = ptr InternalState
ZStream*{.final, pure.} = object
nextIn*: Pbytef
availIn*: Uint
totalIn*: Ulong
nextOut*: Pbytef
availOut*: Uint
totalOut*: Ulong
ZStream*{.final, pure, importc: "z_stream", header: "<zlib.h>".} = object
next_in*: Pbytef
avail_in*: Uint
total_in*: Ulong
next_out*: Pbytef
avail_out*: Uint
total_out*: Ulong
msg*: Pbytef
state*: PInternalState
zalloc*: Allocfunc
Expand Down Expand Up @@ -314,7 +314,7 @@ proc uncompress*(sourceBuf: cstring, sourceLen: Natural; stream=DETECT_STREAM):
# run loop until all input is consumed.
# handle concatenated deflated stream with header.
while true:
z.availIn = (sourceLen - sbytes).int32
z.availIn = Uint(sourceLen - sbytes)

# no more input available
if (sourceLen - sbytes) <= 0: break
Expand Down Expand Up @@ -355,7 +355,7 @@ proc uncompress*(sourceBuf: cstring, sourceLen: Natural; stream=DETECT_STREAM):
if (status == Z_STREAM_END):
# may have another stream concatenated
if z.availIn != 0:
sbytes = sourceLen - z.availIn # add consumed bytes
sbytes = sourceLen - int(z.availIn) # add consumed bytes
if inflateReset(z) != Z_OK: # reset zlib struct and try again
raise newException(ZlibStreamError, "Invalid stream state(" & $status & ") : " & $z.msg)
else:
Expand Down Expand Up @@ -419,7 +419,7 @@ proc inflate*(buffer: var string; stream=DETECT_STREAM): bool {.discardable.} =
## in this case the proc won't modify the buffer.
##
## Returns true if `buffer` was successfully inflated.

var temp = uncompress(addr(buffer[0]), buffer.len, stream)
if temp.len != 0:
swap(buffer, temp)
Expand Down