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

Add 128bit integer support #173

Merged
merged 4 commits into from
Jun 6, 2018
Merged

Add 128bit integer support #173

merged 4 commits into from
Jun 6, 2018

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Jun 2, 2018

From 1.26, u128 and i128 are stable so let's support them(see rust-lang/rust#35118).

This PR allows conversion between rust 128bit integers and Python's long(in python3, int class), via _PyLong_FromByteArray and _PyLong_AsByteArray.

I referenced
https://github.com/python/cpython/blob/master/Include/longobject.h (for python 3)
and
https://github.com/python/cpython/blob/2.7/Include/longobject.h (for python2)
for this PR.

pub fn _PyLong_FromByteArray(
arg1: *const c_uchar,
arg2: size_t,
arg3: c_int, // is_little_endian
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arguments should better be given their original names, such as is_little_endian, instead of arg3.

}
}
impl<'source> FromPyObject<'source> for $rust_type {
#[cfg(target_endian = "little")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the cfg attached to the method will make the compilation fail if when target_endian is big. If I understood the _PyLong_AsByteArray documentation correctly, extract should also work on big endian.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it's just my mistake, thanks.

@@ -138,6 +138,55 @@ macro_rules! int_convert_u64_or_i64 (
)
);

// for 128bit Integers
macro_rules! int_convert_bignum (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That macro and its tests should go in a common file for python 2 and 3. I know there's already some duplication between num2 and num3, but we shouldn't make it worse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there's already some duplication between num2 and num3, but we shouldn't make it worse.

👍


#[test]
#[cfg(not(Py_LIMITED_API))]
fn test_i128_min() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests are great 👍

@konstin
Copy link
Member

konstin commented Jun 5, 2018

Thank you for your pull request! The implementation looks good; I've added comments on some things that should be changed before merging.

@kngwyu
Copy link
Member Author

kngwyu commented Jun 5, 2018

Thanks for review, and now ready to be reviewed again

@konstin
Copy link
Member

konstin commented Jun 6, 2018

Look great now, thank you!

@konstin konstin merged commit 4fc52cf into PyO3:master Jun 6, 2018
@kngwyu kngwyu deleted the 128bitInt branch June 7, 2018 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants