tomaka / hlua

Rust library to interface with Lua
MIT License
510 stars 48 forks source link

allow getting at the Lua state from a Lua object #161

Closed sunshowers closed 7 years ago

sunshowers commented 7 years ago

This is most useful when the caller wants to do low-level operations on the state that aren't supported through the main API.

sunshowers commented 7 years ago

Hey :) not sure if you saw it, but I pushed updated commits. Let me know what you think, thanks.

tomaka commented 7 years ago

I'm really cautious about doing this. Do you actually need the change, or are you just submitting this PR because "why not"?

sunshowers commented 7 years ago

I do actually need the change, yeah. I'm implementing a library on top of this that adds support for interacting with Lua coroutines. I need to perform a number of low-level operations to make that work. At the moment I'm relying on mem::transmute from a LuaContext, but that's not great.

Could you elaborate a bit on what worries you about this?

On Aug 31, 2017 08:18, "tomaka" notifications@github.com wrote:

I'm really cautious about doing this. Do you actually need the change, or are you just submitting this PR because "why not"?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tomaka/hlua/pull/161#issuecomment-326328705, or mute the thread https://github.com/notifications/unsubscribe-auth/AALBijYqaOPXZNtfBvsdEMBsslZA4fu9ks5sds7GgaJpZM4PE_Fv .

tomaka commented 7 years ago

In my opinion if you need to call raw Lua functions, it means that hlua is missing the functionality you need. Exposing the ffi kind of shows that the library fails at its goal.

It also means that it could be dangerous in the future to change the internal behaviour of hlua (eg. change so that some function pushes two elements on the stack instead of one), because it may break code that uses the FFI.

sunshowers commented 7 years ago

Yeah -- completely agreed that it's not a feature most regular users will want to use. I do think that it's mitigated somewhat by the fact that to do anything at all with the ffi you'll need to use unsafe.

My other changes like allowing creation of PushGuards have a similar problem -- if not used carefully, they could cause Rust and Lua to be out of sync. That's why I made those functions unsafe.

But consider that with these changes in place, I was able to build on top of your excellent work and get Lua coroutines working for my use case in just a couple of days and a few hundred lines of code. Without hlua's support I'd still be reimplementing function calls, poorly. 🙂

I'd be happy to put in warnings/documentation in appropriate places to warn regular users off.

On Aug 31, 2017 10:56, "tomaka" notifications@github.com wrote:

In my opinion if you need to call raw Lua functions, it means that hlua is missing the functionality you need. Exposing the ffi kind of shows that the library fails at its goal.

It also means that it could be dangerous in the future to change the internal behaviour of hlua (eg. change so that some function pushes two elements on the stack instead of one), because it may break code that uses the FFI.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tomaka/hlua/pull/161#issuecomment-326374318, or mute the thread https://github.com/notifications/unsubscribe-auth/AALBilT71cMX8XktHAf-bc3Bh2jPN7Jmks5sdvPWgaJpZM4PE_Fv .

sunshowers commented 7 years ago

Hi @tomaka -- did you come to a discussion here? In the worst case I guess I'll keep on using mem::transmute.

tomaka commented 7 years ago

Since I'm still undecided, please add #[doc(hidden)] on both the extern crate and the state_ptr method, so that they exist but are not advertised in the docs. Once that's done I'll merge.

sunshowers commented 7 years ago

Done! Thanks so much for your time dealing with this.

(If you could get a release out after this lands, that would be awesome! I can get rid of all our hacks at that point. :) )