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 abi3 + num_bigint conversion #3198

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Add abi3 + num_bigint conversion #3198

merged 1 commit into from
Jun 2, 2023

Conversation

youknowone
Copy link
Contributor

Fix #3196

Copy link
Contributor Author

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

@davidhewitt Could you give advices? I left questions.

};
py.get_type::<PyLong>()
.call_method("from_bytes", (bytes_obj, "little"), kwargs)
.expect("TODO")
Copy link
Contributor Author

@youknowone youknowone Jun 2, 2023

Choose a reason for hiding this comment

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

How should I handle errors in to_object? Is this not expected to be implemented for abi3 or unwrap can be ok?

Copy link
Member

Choose a reason for hiding this comment

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

For now .expect() is ok , we will eventually improve this in #1813, it's helpful if you could add a // FIXME #1813 or similar here.

@@ -128,6 +166,32 @@ macro_rules! bigint_conversion {
}
}
}

#[cfg(Py_LIMITED_API)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

where will be the proper places to put the limited api implementation? Like now or a new file or module?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the only thing which changes here is the implementation of how to get n_bytes, so I think we can do something like:

let n_bytes = cfg_if::cfg_if! {
    if #[cfg(not(Py_LIMITED_API))] {
        // fast path
    } else { 
        // slow path
    }
}

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! Questions replied to, also this will need a file newsfragments/3198.added.md, I suggest the following contents:

Add support for `num-bigint` feature in combination with `abi3`.

@@ -2,7 +2,7 @@
//
// based on Daniel Grunwald's https://github.com/dgrunwald/rust-cpython

#![cfg(all(feature = "num-bigint", not(any(Py_LIMITED_API))))]
#![cfg(all(feature = "num-bigint"))]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#![cfg(all(feature = "num-bigint"))]
#![cfg(feature = "num-bigint")]

};
py.get_type::<PyLong>()
.call_method("from_bytes", (bytes_obj, "little"), kwargs)
.expect("TODO")
Copy link
Member

Choose a reason for hiding this comment

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

For now .expect() is ok , we will eventually improve this in #1813, it's helpful if you could add a // FIXME #1813 or similar here.

@@ -128,6 +166,32 @@ macro_rules! bigint_conversion {
}
}
}

#[cfg(Py_LIMITED_API)]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the only thing which changes here is the implementation of how to get n_bytes, so I think we can do something like:

let n_bytes = cfg_if::cfg_if! {
    if #[cfg(not(Py_LIMITED_API))] {
        // fast path
    } else { 
        // slow path
    }
}

@youknowone youknowone marked this pull request as ready for review June 2, 2023 07:17
@DataTriny
Copy link
Contributor

DataTriny commented Jun 2, 2023

First time contributor has agreed to the new licensing scheme.

@youknowone
Copy link
Contributor Author

@DataTriny Thank you. #3108 (comment)

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for implementing this!

@davidhewitt
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 2, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit fa949ff into PyO3:main Jun 2, 2023
@youknowone youknowone deleted the abi3-bigint branch June 3, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

features = ["abi3", "num-bigint"] silently ignore "num-bigint"
3 participants