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

gstring: Specify a lifetime for the From<str> impl #411

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
4 participants
@philn
Copy link
Contributor

philn commented Dec 16, 2018

Needed for gtk-rs/gio#177

#[inline]
fn from(s: &'static str) -> Self {
fn from(s: &'a str) -> Self {
GString::Borrowed(s.as_ptr() as *const c_char, s.len())
}
}

This comment has been minimized.

@EPashkin

EPashkin Dec 16, 2018

Member

This is wrong, as we outlive 'a for sure.
Can you add other From with .to_owned() conversion instead, if rust allow?

This comment has been minimized.

@philn

philn Dec 16, 2018

Author Contributor

In addition to impl From<&'static str> for GString? The conflicting impl isn't allowed.

This comment has been minimized.

@EPashkin

EPashkin Dec 16, 2018

Member

Bad, then we need use s.as_bytes().to_vec().into() as in From<Box<str>>.

This comment has been minimized.

@sdroege

sdroege Dec 16, 2018

Member

How did that work without copying before anyway. We just missed a mistake here before :) We would have to copy it anyway because we need to add a '\0' at the end.

This comment has been minimized.

@philn

philn Dec 16, 2018

Author Contributor

Alright, PR updated accordingly (I think 😅)

@@ -206,9 +206,9 @@ impl From<Box<str>> for GString {
}
}

impl From<&'static str> for GString {
impl<'a> From<&'a str> for GString {

This comment has been minimized.

@sdroege

sdroege Dec 16, 2018

Member

You need to add a lifetime to GString too then,otherwise this is unsafe

This comment has been minimized.

@philn

philn Dec 16, 2018

Author Contributor

Hum this would need another gir codegen round it seems

This comment has been minimized.

@philn

philn Dec 16, 2018

Author Contributor

Do you mean like this?

#[derive(Debug)]
pub enum GString<'a> {
    ForeignOwned(CString),
    Borrowed(*const c_char, usize),
    Owned(*mut c_char, usize)
}

impl<'a> GString<'a> {
...
}

This comment has been minimized.

@sdroege

sdroege Dec 16, 2018

Member

Yes, but as we need to copy anyway (to add the NUL terminator), there's not much point in this. The previous code was wrong, it omitted the NUL terminator so to_glib_none() would be broken.

@philn philn force-pushed the philn:master branch from a306301 to c223326 Dec 16, 2018

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 16, 2018

Looks good to me

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 16, 2018

Looks good to me as well! What about you @EPashkin ?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

@philn Thanks
@GuillaumeGomez 👍

@GuillaumeGomez GuillaumeGomez merged commit 2729b78 into gtk-rs:master Dec 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.