-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add support for the number protocol #267
Conversation
NOTE: We now get warnings because of the modularization of the macro system specified in RFC rust-lang/rfcs#1561 However, fixing these warnings is out of scope for this PR.
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.
Generally looks good, thanks!
A couple of them look like they're calling the wrong CPython function. Can you check and fix these?
Now it mostly matches the corresponding magic methods and CAPI calls. In particular the mismatch between 'div_mod' and 'div_floor' has been annoying me in my code ;)
I fixed the two mistakes you noticed and changed the naming to be more consistent with the corresponding magic methods and CAPI calls. |
err::result_from_owned_ptr(py, ffi::PyNumber_MatrixMultiply(self.as_ptr(), other)) | ||
}) | ||
} | ||
#[cfg(any(feature = "python3-4", feature = "python2-sys"))] { |
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.
This feature checks if the user explicitly requested python3-4 as a cargo feature.
It won't be set when python 3.4 is instead selected via PYTHON_SYS_EXECUTABLE
.
Fixing this will require solving #269 (comment) in some way.
/// Throws an exception if unable to perform | ||
/// the conversion. | ||
#[inline] | ||
fn as_int(&self, py: Python) -> PyResult<PyLong> { |
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.
Following the Rust naming conventions, to_int
would be a more appropriate name -- to me, as_
implies the function returns some form of reference.
NOTE: We now get warnings because of the modularization of the macro system
specified in RFC rust-lang/rfcs#1561
However, fixing these warnings is out of scope for this PR.