-
Notifications
You must be signed in to change notification settings - Fork 875
Add conversion for &Cstr and Cstring #5405
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for working on this! I've added a couple of comments that need addressing. Also, I think we should move these imps into a new cstring.rs
file to keep a better overview.
|
||
#[inline] | ||
fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> { | ||
// Convert cstr to &str, it's is safe because cstr is guaranteed to be valid UTF-8 |
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 is not true, a CStr
can safely be constructed from any &[u8]
without interior null bytes an thus may not contain valid utf8.
Instead of unwrap
ping this should use Utf8Error
as its Error
type, to forward the error to the caller.
|
||
#[inline] | ||
fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> { | ||
// Convert cstring to &str, it's safe because cstring is guaranteed to be valid UTF-8 |
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 (and below) should internally make use of the CStr
conversion above
// Check for interior NUL bytes (excluding the final NUL) | ||
for &byte in &bytes_with_nul[..size as usize] { | ||
if byte == 0 { | ||
return Err(PyValueError::new_err("string contains interior NUL bytes")); | ||
} | ||
} |
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.
No need to do this manually, we should make use of CStr::from_bytes_with_nul and just map the error.
// Fallback for older Python versions | ||
use std::ffi::FromBytesWithNulError; | ||
let cow = py_string.to_cow()?; | ||
let bytes = cow.as_bytes(); |
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 don't think this works, since the underlying buffer will not be null terminated. We may have to gate the whole impl under #[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
#[cfg(feature = "experimental-inspect")] | ||
const INPUT_TYPE: &'static str = "str"; | ||
|
||
fn extract(obj: Borrowed<'_, '_, PyAny>) -> Result<Self, Self::Error> { |
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 should make use of the CStr
impl above and just call to_owned
on the result.
No description provided.