-
Notifications
You must be signed in to change notification settings - Fork 122
gencffi: Detect functions returning a Go error #105
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
Conversation
caf55d9
to
166310c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a few nit-picking comments.
(although the one about memory ownership might be far reaching...)
_examples/pyerrors/pyerrors.go
Outdated
// Use of this source code is governed by a BSD-style | ||
// license that can be found in the LICENSE file. | ||
|
||
// pyerrors is a package to test detecting errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/pyerrors is a package to test detecting errors/Package pyerrors holds functions returning an error/
_examples/pyerrors/test.py
Outdated
r = pyerrors.Div(a, b) | ||
print("pyerrors.Div(%d, %d) = %d"% (a, b, r)) | ||
except RuntimeError: | ||
print("Error detected.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would also be interesting to print the error we caught, as well as for which input arguments it failed.
you should also probably add an except Exception
case as a catch all.
case 1: | ||
g.wrapper.Printf("if not _cffi_helper.lib._cgopy_ErrorIsNil(cret):\n") | ||
g.wrapper.Indent() | ||
g.wrapper.Printf("c_err_str = _cffi_helper.lib._cgopy_ErrorString(cret)\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked, but, do you know what's the memory ownership model for C-strings being passed by _cgopy_ErrorString
?
and what is its interaction with ffi.string
?
ie: who's supposed to free
the C-string ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbinet
For about ffi.string
. I am using this because of Py3 string handling.
(Next PR would be about handling python3 string.)
It's kind of utility function, which copies the string value from string pointer and join into python string. (Internally, it uses ctypes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbinet
Here is the reference from CFFI documentation.
Any operation that would in C return a pointer or array or struct type gives you a fresh cdata >object. Unlike the “original” one, these fresh cdata objects don’t have ownership: they are merely references to existing memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
* Detect functions returning a Go error and make them pythonic (raising an Exception). * Add tests for it. Now it only supports 2 return values. CPy2 and CFFI should support more return values. Fixes: go-python#104
@@ -89,6 +89,11 @@ func _cgopy_ErrorString(err error) *C.char { | |||
return C.CString(err.Error()) | |||
} | |||
|
|||
//export _cgopy_FreeCString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbinet
I add this function to free the C-string manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Now it only supports 2 return values.
CPy2 and CFFI should support more return values.
Fixes: #104