jcmoyer / rust-lua53

Lua 5.3 bindings for Rust
MIT License
158 stars 45 forks source link

Use length-based string functions and return &strs #13

Closed SpaceManiac closed 9 years ago

SpaceManiac commented 9 years ago

There are two related sets of changes in this patch:

  1. Use the length-based variants of string functions where available. This reduces allocations and allows embedding nulls in strings.
  2. Return references from string functions rather than owned Strings. This reduces allocations in cases where the returned result is not actually used, and the old behavior can be gained by appending .to_owned() to call sites.
    • Based on the Lua source, typename_of and typename_at return 'static strings.

I think the first point is pretty important, but the second is somewhat opinion-based. Thoughts?

Also, I should point out I here changed to_str to use luaL_tolstring, which converts more types of things to strings, and doesn't edit the stack in-place. Maybe this isn't desired.

jcmoyer commented 9 years ago

Does Lua make any guarantees about how long a reference to a string on the Lua stack is valid for? I think the original design rationale for returning String was so you couldn't shoot yourself in the foot by keeping an invalid reference around.

SpaceManiac commented 9 years ago

The lifetime elision rules tie the lifetime of the returned &strs to the &mut self, meaning keeping the &str around prevents further use of the State, so the reference won't become invalid.

jcmoyer commented 9 years ago

OK, I admit I'm a bit unfamiliar with those rules. If you can resolve the conflicts I'll go ahead and merge this.

SpaceManiac commented 9 years ago

Conflict resolved.

A declaration like pub fn to_str(&mut self, index: Index) -> Option<&str> desugars to pub fn to_str<'a>(&'a mut self, index: Index) -> Option<&'a str>, such that the returned reference is considered an extension of the mutable borrow of the State.

jcmoyer commented 9 years ago

Looks good, thanks!