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

Replace fdopen by conversion to CFILE #52

Closed
wants to merge 1 commit into from
Closed

Replace fdopen by conversion to CFILE #52

wants to merge 1 commit into from

Conversation

lucasb-eyer
Copy link
Collaborator

So it works with your recent commit to julia-lang :)

@timholy
Copy link
Member

timholy commented Dec 19, 2013

Perhaps surprisingly, I'm a little reluctant to do this. I want to keep supporting release 0.2, and as you noticed CFILE support is only in 0.3. I guess I could fork Images into separate branches, but that seems like a pain for future development and may not be worth it for such a simple change.

@lucasb-eyer
Copy link
Collaborator Author

Ah, I didn't know fdopen was in 0.2, since it's missing in the docs and a quick grep for fdopen in the 0.2 tagged sources doesn't show any result. That's actually why I pulled the newest julia and saw your issue over there.

@timholy
Copy link
Member

timholy commented Dec 19, 2013

Rats, it looks like I deleted the definition of fdopen I put in that file. Will fix. Thanks for letting me know!

@lucasb-eyer
Copy link
Collaborator Author

Shall I close this pull request then?

@timholy timholy closed this in 57fd89f Dec 19, 2013
@timholy
Copy link
Member

timholy commented Dec 19, 2013

OK, it should be in master, would you check out and test?

@lucasb-eyer
Copy link
Collaborator Author

Yup, that works thanks. But you should use the systemerror functions for C calls which set errno on failure. You can change it now, or I can make a pull-request sometime later, as I set out to fix it through julia and the julia-libs I use.

@timholy
Copy link
Member

timholy commented Dec 19, 2013

Thanks for the tip! You mean just changing the last line of error(wand::MagickWand) to systemerror(msg, true)?

@lucasb-eyer
Copy link
Collaborator Author

I mean

-    if FILEp == 0
-        error("fdopen failed")
-    end
+    systemerror("fdopen failed", FILEp == 0)

and either

-    ccall(:fseek, Cint, (Ptr{Void}, Clong, Cint), 
-          FILEp, convert(Clong, offset), int32(0)) == 0 ||
-          error("fseek failed")
+    systemerror("fseek failed", ccall(:fseek, Cint, (Ptr{Void}, Clong, Cint), FILEp, convert(Clong, offset), int32(0)) == 0)

or

-    ccall(:fseek, Cint, (Ptr{Void}, Clong, Cint), 
-          FILEp, convert(Clong, offset), int32(0)) == 0 ||
-          error("fseek failed")
+    status = ccall(:fseek, Cint, (Ptr{Void}, Clong, Cint), FILEp, convert(Clong, offset), int32(0))
+    systemerror("fseek failed", status == 0)

@timholy
Copy link
Member

timholy commented Dec 19, 2013

Ah, right. (I had mentally moved on to your other issue.)

I'll be happy to do this, if you prefer, though obviously this was rather close to a PR already!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants