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

Should ToLua take self by reference. #287

Open
kaikalii opened this issue Jun 17, 2023 · 2 comments
Open

Should ToLua take self by reference. #287

kaikalii opened this issue Jun 17, 2023 · 2 comments

Comments

@kaikalii
Copy link
Contributor

kaikalii commented Jun 17, 2023

Basically every implementation of ToLua is not actually going to consume the value, yet ToLua::to_lua (or 0.9 IntoLua::into_lua) takes self by value.
The only time I can think when you might need to move out of self when converting to a mlua::Value is when self contains a lua object in the first place, in which case you can just cheaply clone it.

I am finding I sometimes have to clone more expensive things to convert them to Lua values when that shouldn't be necessary.

Should the method take &self?
Maybe only require Self: ?Sized?

The only downside I can see is that it would be a breaking change, and a minor one to patch at that.

If this is agreeable, I'd be happy to implement it.

@khvzak
Copy link
Member

khvzak commented Jun 17, 2023

The only time I can think when you might need to move out of self when converting to a mlua::Value is when self contains a lua object in the first place, in which case you can just cheaply clone it.

There is one more reason actually:

impl<'lua, T: UserData + MaybeSend + 'static> IntoLua<'lua> for T {
    fn into_lua(self, lua: &'lua Lua) -> Result<Value<'lua>> {
        Ok(Value::UserData(lua.create_userdata(self)?))
    }
}

I was thinking about taking self by reference and keeping the trait name ToLua, but seems having the ability to move T into lua is more valuable.

I am finding I sometimes have to clone more expensive things to convert them to Lua values when that shouldn't be necessary.

What data structures you use? IntoLua is implemented for few reference objects like &[], &str. Maybe adding few more implementations would help?

@kaikalii
Copy link
Contributor Author

Ah I see. I didn't realize &[] implemented IntoLua. I think it would be good if &HashMap and &BTreeMap implemented it (and maybe the *Sets too).

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

No branches or pull requests

2 participants