Impl partialeq for pylong #4317
Conversation
There was a problem hiding this comment.
Thanks for the PR.
However I identify a couple of problems with it. You aren't handling overflow anywhere, which means unequal values can compare equal if overflow (on either side) occurs.
I think it would be better to just extract the PyLong to the compared-to integer type and compare that, rather than casting to c_long.
Finally, you can simplify this with a macro. For example:
macro_rules! int_compare {
($rust_type: ty) => {
impl PartialEq<Bound<'_, PyLong>> for $rust_type {
#[inline]
fn eq(&self, other: &Bound<'_, PyLong>) -> bool {
todo!()
}
}
};
}
int_compare!(i8);
int_compare!(u8);
int_compare!(i16);
//etc
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
| @@ -224,7 +224,7 @@ macro_rules! test_case { | |||
| let struct_obj = struct_class.call0().unwrap(); | |||
| assert!(struct_obj.setattr($renamed_field_name, 2).is_ok()); | |||
| let attr = struct_obj.getattr($renamed_field_name).unwrap(); | |||
| assert_eq!(2, attr.extract().unwrap()); | |||
| assert_eq!(2, attr.extract::<u8>().unwrap()); | |||
There was a problem hiding this comment.
So just completing my read of the full diff, this is interesting.
What I see here is that the introduction of these implementations has changed type inference here. It's a potentially breaking change.
Is it a problem? I don't think so; user code could already define other types implementing FromPyObject and PartialEq<u8> and thus have the same inference failure.
And I believe that this new PartialEq implementation here still holds potential convenience and usability improvements.
There was a problem hiding this comment.
I see how this can be a breaking change. However, I don't think it's a major issue.
Ultimately, I'll leave the final decision to the maintainers haha 😃 .
There was a problem hiding this comment.
Agreed, I think we move ahead with this.
There was a problem hiding this comment.
I don't consider inference failures like this as breaking changes, so I'm also fine with this.
|
I fixed the merge conflict and will proceed to merge, thanks again! |
Thanks for the great work on PyO3!
This PR implements
PartialEqforPyLongwithu8,u16,u32,u64,u128,usize,i8,i16,i32,i64,i128andisize. See #4250As this is my first contribution to the project, I welcome any feedback or suggestions.