-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
I really dislike this API. Can we not make it something like this:
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 |
@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 |
@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 |
Redesigning this interface probably doesn't need to block this pull request though. |
Oh, what I really like the least about this api is that it is possible to read uninitialized memory when checking for null. |
@brson Are you talking about the current as_c_str? how can it read uninitialized memory? |
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.
@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. |
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.