Improved error handling in base #5299

Merged
merged 7 commits into from Jan 22, 2014

2 participants

@lucasb-eyer

As a follow-up to #5193, I went ahead and checked the error-handling of most ccalls in base/. This pull-request does the following (I suggest to read the commits one-by-one instead of the full diff):

  • a35ee0e replaces calls to error by calls to systemerror where these match perfectly.
  • 4ff219f handles two cases where the ccalls might fail in two different ways, one with setting errno and the other without.
  • b4e722d adds calls to systemerror where there weren't but I think there should be.
  • 8af88ab fixes a bunch of minor errors and inconsistencies with error-handling in fs.jl.

I've been very cautious and make testall passes all tests, but please do review and comment.

@JeffBezanson JeffBezanson commented on an outdated diff Jan 4, 2014
@@ -232,8 +231,12 @@ const SEEK_SET = int32(0)
const SEEK_CUR = int32(1)
const SEEK_END = int32(2)
-position(f::File) = ccall(:jl_lseek, Coff_t,(Int32,Coff_t,Int32),f.handle,0,SEEK_CUR)
-
+function position(f::File)
+ ret = ccall(:jl_lseek, Coff_t,(Int32,Coff_t,Int32),f.handle,0,SEEK_CUR)
+ systemerror("lseek", ret == -one(Coff_t))
@JeffBezanson
The Julia Language member
JeffBezanson added a line comment Jan 4, 2014

This can be -1. We're not perfect, but numeric equality does work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JeffBezanson JeffBezanson and 1 other commented on an outdated diff Jan 4, 2014
@@ -266,39 +266,43 @@ show(io::IO, s::IOStream) = print(io, "IOStream(", s.name, ")")
fd(s::IOStream) = int(ccall(:jl_ios_fd, Clong, (Ptr{Void},), s.ios))
close(s::IOStream) = ccall(:ios_close, Void, (Ptr{Void},), s.ios)
isopen(s::IOStream) = bool(ccall(:ios_isopen, Cint, (Ptr{Void},), s.ios))
-flush(s::IOStream) = ccall(:ios_flush, Void, (Ptr{Void},), s.ios)
+flush(s::IOStream) = systemerror("flush", ccall(:ios_flush, Void, (Ptr{Void},), s.ios) != 0)
@JeffBezanson
The Julia Language member
JeffBezanson added a line comment Jan 4, 2014

The return type of ios_flush should be Cint.

@lucasb-eyer
lucasb-eyer added a line comment Jan 4, 2014

Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JeffBezanson JeffBezanson commented on the diff Jan 4, 2014
src/support/ios.c
@@ -405,7 +405,7 @@ off_t ios_seek(ios_t *s, off_t pos)
s->_eof = 0;
if (s->bm == bm_mem) {
if ((size_t)pos > s->size)
- return -1;
+ return -2;
@JeffBezanson
The Julia Language member
JeffBezanson added a line comment Jan 4, 2014

Why?

@JeffBezanson
The Julia Language member
JeffBezanson added a line comment Jan 4, 2014

Ah, just found the note explaining it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lucasb-eyer

Rebased to master since there was a conflicting update. (I hope I didn't screw that up, first time I've done it on github.)

Also added a comments in the sourcecode about the -1/-2 return values, since it isn't obvious at all and PRs aren't good documentation.

@StefanKarpinski StefanKarpinski merged commit 514eb69 into JuliaLang:master Jan 22, 2014

1 check passed

Details default The Travis CI build passed
@tchajed tchajed referenced this pull request in JuliaLang/IJulia.jl Jan 24, 2014
Closed

Cannot import modules with latest Julia #137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment