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 as_c_str method on strings #6729

Merged
merged 1 commit into from
Jun 1, 2013
Merged

Add as_c_str method on strings #6729

merged 1 commit into from
Jun 1, 2013

Conversation

bstrie
Copy link
Contributor

@bstrie bstrie commented May 25, 2013

Formerly this was a free function rather than a method. I've left it in place for now, although redefined it so that it just calls the method.

@pcwalton
Copy link
Contributor

I really dislike this API. Can we not make it something like this:

enum CStringBundle {
    allocation: Option<~[u8]>,
    c_str: *u8,
}

fn as_c_str(&self) -> CStringBundle {
    if I'm null terminated {
        CStringBundle { None, &self[0] }
    } else {
        let new_allocation = copy and null terminate me;
        CStringBundle { Some(new_allocation), &new_allocation[0] }
    } 
}

HELLO WORLD LOL, THIS IS BEN EDITING PATRICK'S COMMENT TO PROVE THAT GITHUB ALLOWS ME TO EDIT ANY ARBITRARY COMMENT ON ANY PULL REQUEST THAT I'VE OPENED! NOTHING IS SAFE!

R+ BSTRIE/REFLECTIONS_ON_TRUSTING_TRUST

@bstrie
Copy link
Contributor Author

bstrie commented May 25, 2013

@Aatch Good catch, I completely glossed over the extra indirection there.

@pcwalton I'm not attached to the current API, but how would you expect to use a CStringBundle for C interop? Right now my Lua interop code looks like this:

let ret1 = str::as_c_str("function foo (x,y) return x+y end",
                         |s| luaL_loadstring(L, s));

With this pull I was expecting to change it to this:

let ret1 = do "function foo (x,y) return x+y end".as_c_str |s| {
    luaL_loadstring(L, s);
}

This still seems roundabout, I'd be perfectly happy with something like:

let ret1 = luaL_loadstring(L, "function foo (x,y) return x+y end".as_c_str());

With your proposed function, would "foo".as_c_str() become "foo".as_c_str().c_str ?

@brson
Copy link
Contributor

brson commented May 26, 2013

@pcwalton What I don't like about this API most is that the performance characteristics are not obvious. What if we instead changed the signature to as_c_str(&~str) so you are guaranteed to be null terminated and not allocate?

@brson
Copy link
Contributor

brson commented May 26, 2013

Redesigning this interface probably doesn't need to block this pull request though.

@brson
Copy link
Contributor

brson commented May 26, 2013

Oh, what I really like the least about this api is that it is possible to read uninitialized memory when checking for null.

@Kimundi
Copy link
Member

Kimundi commented May 26, 2013

@brson Are you talking about the current as_c_str? how can it read uninitialized memory?

@bstrie
Copy link
Contributor Author

bstrie commented May 31, 2013

I've opened #6869 for addressing the API change. In the meantime, this is still the direction that we'd like to go wrt changing functions to methods where it makes sense (#6045).

bors added a commit that referenced this pull request Jun 1, 2013
Formerly this was a free function rather than a method. I've left it in place for now, although redefined it so that it just calls the method.
@bors bors closed this Jun 1, 2013
@bors bors merged commit c77d58f into rust-lang:incoming Jun 1, 2013
@brson
Copy link
Contributor

brson commented Jun 1, 2013

@Kimundi perhaps it can't if we maintain the invariant that all slices are overallocated. There was a bug once because some code didn't and I've had a vendetta for it since.

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.

6 participants