-
Notifications
You must be signed in to change notification settings - Fork 802
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 c_str!
macro to create &'static CStr
#4255
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.
I'm a big fan of this :)
@@ -870,7 +870,8 @@ pub enum PropertyType<'a> { | |||
} | |||
|
|||
impl PropertyType<'_> { | |||
fn null_terminated_python_name(&self) -> Result<syn::LitStr> { | |||
fn null_terminated_python_name(&self, ctx: &Ctx) -> Result<TokenStream> { |
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.
Rather than using TokenStream
I'd prefer we make our own type for this and switch to syn::LitCStr
when it becomes available.
Then we can also change
pub struct PythonDoc(TokenStream);
to
pub struct LitCStr { .. };
pub struct PythonDoc(LitCStr);
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'm happy to go with that; I might also see how painful it is to use conditional code to emit c""
directly from macros on new enough versions. I think it'd be much more efficient codegen and makes it easier for us to clean up later.
... but maybe in the interest of not blocking the 0.22 release too much, would you be ok if I punt doing this into a follow-up internal refactoring 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.
Sure 👍
@@ -284,22 +272,6 @@ where | |||
retval | |||
} | |||
|
|||
pub(crate) struct PyMethodDefDestructor { |
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'm happy to get rid of this :)
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.
Me too! It felt so awkward before to have to do the dance with &'static str
in a bunch of places. In some we created these destructors and in others we made an unsafe requirement of no interior nulls. This end state is much more satisfying.
Co-authored-by: Bruno Kolenbrander <[email protected]>
Big 👍 for this from me as well! This is a lot nicer and safer than having to remember to add the |
Given the two broadly positive sentiments, I think I'm going to merge this once I get CI passing. It'll make #4254 easier to finish implementing, which I want to complete ASAP with a view to then shipping 0.22. If there are changes which are wanted (like the |
With Rust 1.77 finally stabilising
c""
c-string literals, I've wanted for a little while to be able to make better use of&'static CStr
inside PyO3.This PR proposes a pathway to doing that, by adding a
c_str!("foo")
macro to create a static c-string safely. With MSRV 1.63 we're able to support this on all versions.This has the nice effect of moving work from runtime to compile time, because at the moment we have a lot of
&str
which need to be converted to&CStr
using runtime checks. In the future when MSRV passes 1.77 we have a trivial cleanup to swap everything toc""
literals.The only user-facing changes:
pyo3_ffi::c_str
macro (also accessible aspyo3::ffi::c_str
).PyCFunction
constructors now take&'static CStr
where they previously took&'static str
.