luajit/lua51: fix loadFile and checkVersion #79
Conversation
The luajit and lua51 internal API didn't take a `mode` variable, meaning the function signature was different between these two versions and lua 5.2+. Add an unused `Mode` to the loadFile51 signature to allow building with luajit/lua51 The checkVersion function calls luaL_checkversion, which was only introduced in lua 5.2. Add an empty call when using a version less than 5.2
|
Hey thank you for this contribution! So I'm actually unsure how I want to address this. From the perspective of making the API consistent across all Lua versions this is a good fix. But I'm not sure if that is the direction I want to take ziglua. For example, you fixed loadFile and checkVersion, but there are many other functions with inconsistencies between the Lua versions. Currently the approach to handle this is to use |
|
On Sat Apr 20, 2024 at 6:35 PM AKDT, Nathan Craddock wrote:
Hey thank you for this contribution! So I'm actually unsure how I want to
address this. From the perspective of making the API consistent across all Lua
versions this is a good fix. But I'm not sure if that is the direction I want
to take ziglua.
Completely understand!
Currently the approach to handle this is to use `ziglua.lang` in your code to
handle any cases where a function differs or isn't available. Would this be a
reasonable approach for you? I'm happy to discuss this further to find other
solutions
I'm not sure if this addresses my issue. From a basic use case, i want to use
luajit and call "doFile". To do this, I think the most simple reproducer is:
```zig
const std = @import("std");
const ziglua = @import("ziglua");
const Lua = ziglua.Lua;
pub fn main() anyerror!void {
var gpa = std.heap.GeneralPurposeAllocator(.{}){};
const allocator = gpa.allocator();
defer _ = gpa.deinit();
var lua = try Lua.init(&allocator);
defer lua.deinit();
try lua.doFile("foo.lua");
}
```
This doesn't work in lua51 or luajit because `doFile` passes two parameters to
`lua.loadFile`. Is the intention that I should do something like:
```zig
const std = @import("std");
const ziglua = @import("ziglua");
const Lua = ziglua.Lua;
pub fn main() anyerror!void {
var gpa = std.heap.GeneralPurposeAllocator(.{}){};
const allocator = gpa.allocator();
defer _ = gpa.deinit();
var lua = try Lua.init(&allocator);
defer lua.deinit();
switch (ziglua.lang) {
.luajit, .lua51 => {
try lua.loadFile("foo.lua");
try lua.protectedCall(0, ziglua.mult_return, 0);
},
else => try lua.doFile("foo.lua"),
}
}
```
I can certainly do this in my code since I know which version I am using and can
avoid calling doFile directly, which let's me bypass the switch and leaves me
with a net +1 LOC - no big deal for me.
I think where this is confusing is that lua 5.1 *does* have a `dofile` method
which takes a single param, but this api is not currently accessible in ziglua.
I fully understand if that is your design choice, I'd just petition that you
reconsider making this API available for lua51/luajit :).
Reference: https://www.lua.org/manual/5.1/manual.html#pdf-dofile
…--
Tim
|
|
I see it wasn't quite clear the trace I was calling to get to why I fixed
`loadFile`. I am calling `dofile("foo.lua")` in luajit, which in turn calls
`loadFile` with two parameters. Perhaps the fix for this should actually be a
switch in the `doFile` function for how it calls `loadFile`:
```zig
/// Loads and runs the given file
/// See https://www.lua.org/manual/5.4/manual.html#luaL_dofile
pub fn doFile(lua: *Lua, file_name: [:0]const u8) !void {
// translate-c failure
switch (lang) {
.luajit, .lua51 => try lua.loadFile(file_name),
else => try lua.loadFile(file_name, .binary_text),
}
try lua.protectedCall(0, mult_return, 0);
}
```
…--
Tim
|
|
I guess email replies don't work well with code formatting. I think the second fix keeps the ziglua 5.1 api the same as lua5.1 (one param for loadFile) and also allows lua5.1 users to call |
|
Hey there! So sorry for leaving this hanging so long. I've been super swamped the last month. I'll take a closer look later today / over the next few days and get back to you |
doFile didn't properly handle different versions See #79
/// Loads and runs the given file
/// See https://www.lua.org/manual/5.4/manual.html#luaL_dofile
pub fn doFile(lua: *Lua, file_name: [:0]const u8) !void {
// translate-c failure
switch (lang) {
.luajit, .lua51 => try lua.loadFile(file_name),
else => try lua.loadFile(file_name, .binary_text),
}
try lua.protectedCall(0, mult_return, 0);
}It took me reading this PR several times, and the source code to comlink to finally figure it out what you explained so simply... sometimes reading is hard 😂 I just merged a fix with this above suggested approach. Thanks! Thanks for raising this issue, and sorry for the delay! Also I'm leaving checkVersion untouched, but feel free to open an issue or PR if you think there is something to be done there. |
The luajit and lua51 internal API didn't take a
modevariable, meaningthe function signature was different between these two versions and lua
5.2+. Add an unused
Modeto the loadFile51 signature to allow buildingwith luajit/lua51
The checkVersion function calls luaL_checkversion, which was only
introduced in lua 5.2. Add an empty call when using a version less than
5.2