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

Luau support #30

Closed
Tracked by #32
natecraddock opened this issue Dec 1, 2023 · 15 comments · Fixed by #36
Closed
Tracked by #32

Luau support #30

natecraddock opened this issue Dec 1, 2023 · 15 comments · Fixed by #36
Labels
feature New feature or request
Milestone

Comments

@natecraddock
Copy link
Owner

Some conversations on #19 show interest in Luau support.

It should be possible, though I haven't attempted yet. It is written in C++ which may cause issues.

@nurpax
Copy link
Contributor

nurpax commented Dec 1, 2023

I tried doing this and got it to work. I built Luau using their cmake-based build and linked the output library with ziglua.

It'd be better if everything was built through build.zig but I didn't look into that part. It might be doable or it might get hairy.

I can share my notes if you want, although I didn't encounter any major problems. You need to configure the cmake build correctly to make it use extern "C" to make it work.

There are some differences between Luau and other Lua implementations and Luau also uses a compiler and code is loaded as bytecode so it'll need some additional functions and tweaks. But the usual stuff like calling a Lua function uses the same Lua 5.1 API.

@natecraddock
Copy link
Owner Author

Thanks for taking a look into things! Feel free to share your notes here, that would likely be helpful.

I'm also interested in getting things to work with the build.zig. I'm planning on working on this over the next few days. I expect it should be possible because mlua seems to support Luau bindings for Rust

@nurpax
Copy link
Contributor

nurpax commented Dec 2, 2023

Here's the mods that I made to ziglua to compile against Luau. I don't think there's anything that's directly useful but sharing here in case it's helpful:

diff --git a/build.zig b/build.zig
index ccb249e..8f1f867 100644
--- a/build.zig
+++ b/build.zig
@@ -8,6 +8,7 @@ pub const LuaVersion = enum {
     lua_53,
     lua_54,
     // lua_jit,
+    luau,
 };
 
 pub fn build(b: *Build) void {
@@ -27,6 +28,7 @@ pub fn build(b: *Build) void {
             .lua_52 => .{ .path = "src/ziglua-5.2/lib.zig" },
             .lua_53 => .{ .path = "src/ziglua-5.3/lib.zig" },
             .lua_54 => .{ .path = "src/ziglua-5.4/lib.zig" },
+            .luau => .{ .path = "src/zigluau/lib.zig" },
         },
     });
 
@@ -40,6 +42,7 @@ pub fn build(b: *Build) void {
             .lua_52 => std.SemanticVersion{ .major = 5, .minor = 2, .patch = 4 },
             .lua_53 => std.SemanticVersion{ .major = 5, .minor = 3, .patch = 6 },
             .lua_54 => std.SemanticVersion{ .major = 5, .minor = 4, .patch = 6 },
+            .luau => std.SemanticVersion{ .major = 1, .minor = 0, .patch = 0 },
         },
     };
     const lib = if (shared)
@@ -51,8 +54,10 @@ pub fn build(b: *Build) void {
         .lua_51 => "lib/lua-5.1/src",
         .lua_52 => "lib/lua-5.2/src",
         .lua_53 => "lib/lua-5.3/src",
-        .lua_54 => "lib/lua-5.4/src",
+        .lua_54 => "lib/lua5.4/src",
+        .luau => "../luau/VM/include",
     };
+
     lib.addIncludePath(.{ .path = lib_dir });
 
     const os_tag = target.os_tag orelse
@@ -79,16 +84,18 @@ pub fn build(b: *Build) void {
         .lua_52 => &lua_52_source_files,
         .lua_53 => &lua_53_source_files,
         .lua_54 => &lua_54_source_files,
+        .luau => &luau_source_files,
     };
     for (lua_source_files) |file| {
-        const path = std.fs.path.join(b.allocator, &.{ lib_dir, file }) catch unreachable;
+        const dummy_lib_dir = "lib/luau/src";
+        const path = std.fs.path.join(b.allocator, &.{ dummy_lib_dir, file }) catch unreachable;
         lib.addCSourceFile(.{ .file = .{ .path = path }, .flags = &flags });
     }
-    lib.linkLibC();
+    //lib.linkLibC();
 
     lib.installHeader(b.pathJoin(&.{ lib_dir, "lua.h" }), "lua/lua.h");
     lib.installHeader(b.pathJoin(&.{ lib_dir, "lualib.h" }), "lua/lualib.h");
-    lib.installHeader(b.pathJoin(&.{ lib_dir, "lauxlib.h" }), "lua/lauxlib.h");
+    //lib.installHeader(b.pathJoin(&.{ lib_dir, "lauxlib.h" }), "lua/lauxlib.h");
     lib.installHeader(b.pathJoin(&.{ lib_dir, "luaconf.h" }), "lua/luaconf.h");
 
     b.installArtifact(lib);
@@ -100,6 +107,7 @@ pub fn build(b: *Build) void {
             .lua_52 => .{ .path = "src/ziglua-5.2/tests.zig" },
             .lua_53 => .{ .path = "src/ziglua-5.3/tests.zig" },
             .lua_54 => .{ .path = "src/ziglua-5.4/tests.zig" },
+            .luau => .{ .path = "src/zigluau/tests.zig" },
         },
         .optimize = optimize,
     });
@@ -123,7 +131,13 @@ pub fn build(b: *Build) void {
             .optimize = optimize,
         });
         exe.addModule("ziglua", ziglua);
+
+        exe.addLibraryPath(.{ .path = "../luau/cmake/RelWithDebInfo/" });
+        exe.linkSystemLibraryName("Luau.VM");
+
         exe.linkLibrary(lib);
+        exe.linkLibC();
+        //exe.linkLibCpp();
 
         const artifact = b.addInstallArtifact(exe, .{});
         const exe_step = b.step(b.fmt("install-example-{s}", .{example[0]}), b.fmt("Install {s} example", .{example[0]}));
@@ -275,3 +289,5 @@ const lua_54_source_files = [_][]const u8{
     "lutf8lib.c",
     "linit.c",
 };
+
+const luau_source_files = [_][]const u8{"ldummy.c"};
diff --git a/examples/zig-fn.zig b/examples/zig-fn.zig
index 6674bb9..786f6ea 100644
--- a/examples/zig-fn.zig
+++ b/examples/zig-fn.zig
@@ -10,8 +10,8 @@ const Lua = ziglua.Lua;
 // A Zig function called by Lua must accept a single *Lua parameter and must return an i32.
 // This is the Zig equivalent of the lua_CFunction typedef int (*lua_CFunction) (lua_State *L) in the C API
 fn adder(lua: *Lua) i32 {
-    const a = lua.toInteger(1) catch 0;
-    const b = lua.toInteger(2) catch 0;
+    const a = lua.toInteger(1); // catch 0;
+    const b = lua.toInteger(2); // catch 0;
     lua.pushInteger(a + b);
     return 1;
 }
@@ -31,28 +31,28 @@ pub fn main() anyerror!void {
     // The call to ziglua.wrap() is slightly more verbose, but has the benefit of being more flexible.
     lua.pushFunction(ziglua.wrap(adder));
 
-    // Push the arguments onto the stack
+    // // Push the arguments onto the stack
     lua.pushInteger(10);
     lua.pushInteger(32);
 
-    // Call the function. It accepts 2 arguments and returns 1 value
+    // // Call the function. It accepts 2 arguments and returns 1 value
     // We use catch unreachable because we can verify this function call will not fail
     lua.protectedCall(2, 1, 0) catch unreachable;
 
-    // The result of the function call is on the stack.
-    // Use toInteger to read the integer at index 1
-    std.debug.print("the result: {}\n", .{lua.toInteger(1) catch unreachable});
+    // // The result of the function call is on the stack.
+    // // Use toInteger to read the integer at index 1
+    std.debug.print("the result: {}\n", .{lua.toInteger(1)});
 
-    // We can also register the function to a global and run from a Lua "program"
+    // // We can also register the function to a global and run from a Lua "program"
     lua.pushFunction(ziglua.wrap(adder));
     lua.setGlobal("add");
 
-    // We need to open the base library so the global print() is available
-    lua.open(.{ .base = true });
+    // // We need to open the base library so the global print() is available
+    // lua.open(.{ .base = true });
 
-    // Our "program" is an inline string
-    lua.doString(
-        \\local sum = add(10, 32)
-        \\print(sum)
-    ) catch unreachable;
+    // // Our "program" is an inline string
+    // lua.doString(
+    //     \\local sum = add(10, 32)
+    //     \\print(sum)
+    // ) catch unreachable;
 }

src/luau diff against src/ziglua-5.1:

diff -u ziglua-5.1/lib.zig zigluau/lib.zig
--- ziglua-5.1/lib.zig  1900-01-00 00:00:00 +0000
+++ zigluau/lib.zig     1900-01-00 00:00:00 +0000
@@ -6,7 +6,6 @@
 const c = @cImport({
     @cInclude("lua/lua.h");
     @cInclude("lua/lualib.h");
-    @cInclude("lua/lauxlib.h");
 });

 const Allocator = std.mem.Allocator;
@@ -95,14 +94,6 @@
     File,
 };

-/// The type of event that triggers a hook
-pub const Event = enum(u3) {
-    call = c.LUA_HOOKCALL,
-    ret = c.LUA_HOOKRET,
-    line = c.LUA_HOOKLINE,
-    count = c.LUA_HOOKCOUNT,
-};
-
 /// Type for arrays of functions to be registered
 pub const FnReg = struct {
     name: [:0]const u8,
@@ -688,7 +679,7 @@
     /// `n` tells how many upvalues this function will have
     /// See https://www.lua.org/manual/5.1/manual.html#lua_pushcclosure
     pub fn pushClosure(lua: *Lua, c_fn: CFn, n: i32) void {
-        c.lua_pushcclosure(lua.state, c_fn, n);
+        c.lua_pushcclosurek(lua.state, c_fn, null, n, null); // TODO expose debug name
     }

     /// Pushes a function onto the stack.
@@ -896,7 +887,7 @@
     /// The Lua value must be an integer, or a number, or a string convertible to an integer otherwise toInteger returns 0
     /// See https://www.lua.org/manual/5.1/manual.html#lua_tointeger
     pub fn toInteger(lua: *Lua, index: i32) Integer {
-        return c.lua_tointeger(lua.state, index);
+        return c.lua_tointegerx(lua.state, index, null);
     }

     /// Returns a slice of bytes at the given index
@@ -1307,21 +1298,6 @@
         }
     }

-    /// Loads a string as a Lua chunk
-    /// See https://www.lua.org/manual/5.1/manual.html#luaL_loadstring
-    pub fn loadString(lua: *Lua, str: [:0]const u8) !void {
-        const ret = c.luaL_loadstring(lua.state, str.ptr);
-        switch (ret) {
-            StatusCode.ok => return,
-            StatusCode.err_syntax => return error.Syntax,
-            StatusCode.err_memory => return error.Memory,
-            // loadstring runs lua_load which runs pcall, so can also return any result of an pcall error
-            StatusCode.err_runtime => return error.Runtime,
-            StatusCode.err_error => return error.MsgHandler,
-            else => unreachable,
-        }
-    }
-
     /// If the registry already has the key `key`, returns an error
     /// Otherwise, creates a new table to be used as a metatable for userdata
     /// See https://www.lua.org/manual/5.1/manual.html#luaL_newmetatable
@@ -1575,7 +1551,6 @@
 }

 pub const ZigFn = fn (lua: *Lua) i32;
-pub const ZigHookFn = fn (lua: *Lua, event: Event, info: *DebugInfo) void;
 pub const ZigContFn = fn (lua: *Lua, status: Status, ctx: i32) i32;
 pub const ZigReaderFn = fn (lua: *Lua, data: *anyopaque) ?[]const u8;
 pub const ZigWriterFn = fn (lua: *Lua, buf: []const u8, data: *anyopaque) bool;
@@ -1584,7 +1559,6 @@
     return switch (T) {
         LuaState => Lua,
         ZigFn => CFn,
-        ZigHookFn => CHookFn,
         ZigReaderFn => CReaderFn,
         ZigWriterFn => CWriterFn,
         else => @compileError("unsupported type given to wrap: '" ++ @typeName(T) ++ "'"),
@@ -1599,7 +1573,6 @@
     return switch (T) {
         LuaState => Lua{ .state = value },
         ZigFn => wrapZigFn(value),
-        ZigHookFn => wrapZigHookFn(value),
         ZigReaderFn => wrapZigReaderFn(value),
         ZigWriterFn => wrapZigWriterFn(value),
         else => @compileError("unsupported type given to wrap: '" ++ @typeName(T) ++ "'"),
@@ -1616,21 +1589,6 @@
         }
     }.inner;
 }
-
-/// Wrap a ZigHookFn in a CHookFn for passing to the API
-fn wrapZigHookFn(comptime f: ZigHookFn) CHookFn {
-    return struct {
-        fn inner(state: ?*LuaState, ar: ?*Debug) callconv(.C) void {
-            // this is called by Lua, state should never be null
-            var lua: Lua = .{ .state = state.? };
-            var info: DebugInfo = .{
-                .current_line = if (ar.?.currentline == -1) null else ar.?.currentline,
-                .private = ar.?.i_ci,
-            };
-            @call(.always_inline, f, .{ &lua, @as(Event, @enumFromInt(ar.?.event)), &info });
-        }
-    }.inner;
-}

 /// Wrap a ZigReaderFn in a CReaderFn for passing to the API
 fn wrapZigReaderFn(comptime f: ZigReaderFn) CReaderFn {

I had to use the following zig build -Dtarget=x86_64-windows-msvc --verbose -Dversion=luau --summary none run-example-zig-function to compile Ziglua on Windows to make it link against Luau. Most likely this is not required if you use build.zig to build Luau. I ran into some linker problems due to CRT mismatches.

Building Luau:

mkdir cmake && cd cmake
cmake .. -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLUAU_EXTERN_C=1
cmake --build . --target Luau.Repl.CLI --config RelWithDebInfo
cmake --build . --target Luau.Analyze.CLI --config RelWithDebInfo

I'm not sure how Ziglua should deal with the Luau command line tools. Probably wouldn't be a bad idea to also package & build those through build.zig? More work though. The compiler is something that I believe is a must use with Luau, so perhaps building that along with everything else in Ziglua will be pretty convenient.

BTW fingers crossed that Zig dropping LLVM (and thus C++) won't make bundling C++ libs like Luau a tool configuration nightmare.

@natecraddock
Copy link
Owner Author

Hey! Sorry for the weeks of silence. I finally had a large block of time to work on this.

I have Luau building from source in the build.zig file now, and tests seem to be passing. I did have to comment out several tests in my rush to get things working. I'll take some time now to clean things up.

@nurpax
Copy link
Contributor

nurpax commented Dec 23, 2023

Really cool, thanks for the update, @natecraddock! Looking forward to using it.

@natecraddock
Copy link
Owner Author

BTW fingers crossed that Zig dropping LLVM (and thus C++) won't make bundling C++ libs like Luau a tool configuration nightmare.

Missed this the first time. My understanding is that there would be some sort of optional llvm dependency still. It's a long ways out though, so only time will tell!

I just pushed to the luau branch (https://github.com/natecraddock/ziglua/tree/luau) all my changes. It is still a work in progress, but the basics should work now. Feel free to give it a try if you have time. I just tested using Luau with the build system and it seems to work great!

const ziglua = b.dependency("ziglua", .{
    .target = target,
    .optimize = optimize,
    .version = .luau,
});

This is the first time I have used Luau, so I'm unfamiliar with what is expected. If I have missed anything let me know!

@nurpax
Copy link
Contributor

nurpax commented Dec 24, 2023

Thanks, I'll give it a try over the holidays! I actually haven't used Luau either, but I have a strong intuition that I'll have a better time with it than normal Lua. Luau has some non-Roblox users too, such as Alan Wake 2 (https://www.remedygames.com/article/how-northlight-makes-alan-wake-2-shine)

@natecraddock natecraddock added the feature New feature or request label Dec 29, 2023
@natecraddock natecraddock mentioned this issue Dec 29, 2023
10 tasks
@natecraddock
Copy link
Owner Author

I just finished fixing all of the tests for Luau. I copied the tests from Lua 5.1, so I may be missing some things that Luau includes from newer versions of Lua. But it should be mostly feature complete now!

https://github.com/natecraddock/ziglua/tree/luau

I'll be merging this in in the next few days after some more testing

@nurpax
Copy link
Contributor

nurpax commented Dec 31, 2023

I wonder if I'm running the tests correctly. Here's what I get when running zig build test -Dversion=luau on Windows:

C:\Users\janne\dev\ziglua>zig build test -Dversion=luau
run test: error: while executing test 'test.executing string contents', the following command exited with code 116 (expected exited with code 0):
C:\Users\janne\dev\ziglua\zig-cache\o\627ce074ec51cf409fcabfed396ffc8f\test.exe --listen=-
Build Summary: 6/8 steps succeeded; 1 failed; 37/37 tests passed (disable with --summary none)
test transitive failure
└─ run test failure
error: the following build command failed with exit code 1:
C:\Users\janne\dev\ziglua\zig-cache\o\011c71f4d84e850171870573012f61d4\build.exe C:\Users\janne\scoop\apps\zig-dev\0.12.0-dev.1808\zig.exe C:\Users\janne\dev\ziglua C:\Users\janne\dev\ziglua\zig-cache C:\Users\janne\AppData\Local\zig --seed 0x7b77de03 test -Dversion=luau

(There's something to be said about the clarity if "zig test" output clarity.. like which actual test failed?)

I think it's failing on loadString:

    try lua.loadString("f = function(x) return x + 10 end");

On Linux, here's what I get:

$ zig build -Dversion=luau test
$

Ie., prints nothing. Who knows what it ran as Zig test system has a tendency of taking overt control over running tests. EDIT: actually I guess it runs something? I ran the produced test binary from the zig cache dir, and that prints All 37 tests passed.. I'll try updating my zig, maybe I have a bad version.

Trying to run example zig function on Linux:

$ zig build -Dversion=luau run-example-zig-function

Build Summary: 7/10 steps succeeded; 1 failed (disable with --summary none)
run-example-zig-function transitive failure
└─ run zig-function transitive failure
   └─ zig build-exe zig-function Debug native 1 errors
examples/zig-fn.zig:32:8: error: member function expected 2 argument(s), found 1
    lua.pushFunction(ziglua.wrap(adder));
    ~~~^~~~~~~~~~~~~
src/zigluau/lib.zig:610:9: note: function declared here
    pub fn pushFunction(lua: *Lua, c_fn: CFn, name: [:0]const u8) void {
    ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
    callMain: /home/janne/zig/lib/std/start.zig:583:32
    initEventLoopAndCallMain: /home/janne/zig/lib/std/start.zig:517:34
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

The interpreter example works tho.

I also didn't look very hard yet, but is there a Zig API for loading bytecode? I think the common approach to using Luau is to compile luau code into bytecode and then load bytecode only.

@nurpax
Copy link
Contributor

nurpax commented Dec 31, 2023

OK, here's a patch that fixes the error on Windows. AFAICT the problem is that it's not portable to assume that std.heap.c_allocator.free() is the same as C lib's free(). It makes sense looking at the implementation of the c_allocator in Zig: https://github.com/ziglang/zig/blob/4129996211edd30b25c23454520fd78b2a70394b/lib/std/heap.zig#L93

C:\Users\janne\dev\ziglua>git diff
diff --git a/lib/luau/Compiler/include/luacode.h b/lib/luau/Compiler/include/luacode.h
index 60cdeee..727d746 100644
--- a/lib/luau/Compiler/include/luacode.h
+++ b/lib/luau/Compiler/include/luacode.h
@@ -40,3 +40,4 @@ struct lua_CompileOptions

 // compile source to bytecode; when source compilation fails, the resulting bytecode contains the encoded error. use free() to destroy
 LUACODE_API char* luau_compile(const char* source, size_t size, lua_CompileOptions* options, size_t* outsize);
+LUACODE_API void luau_free(char* ptr);
diff --git a/lib/luau/Compiler/src/lcode.cpp b/lib/luau/Compiler/src/lcode.cpp
index ee150b1..4412b51 100644
--- a/lib/luau/Compiler/src/lcode.cpp
+++ b/lib/luau/Compiler/src/lcode.cpp
@@ -27,3 +27,8 @@ char* luau_compile(const char* source, size_t size, lua_CompileOptions* options,
     *outsize = result.size();
     return copy;
 }
+
+// free ptr returned by luau_compile
+void luau_free(char* ptr) {
+    free(ptr);
+}
diff --git a/src/zigluau/lib.zig b/src/zigluau/lib.zig
index 4c64f6a..40e7519 100644
--- a/src/zigluau/lib.zig
+++ b/src/zigluau/lib.zig
@@ -1139,7 +1139,8 @@ pub const Lua = struct {
         if (bytecode == null) return error.Memory;

         // luau_compile uses libc malloc to allocate the bytecode on the heap
-        defer std.heap.c_allocator.free(bytecode[0..size]);
+        //defer std.heap.c_allocator.free(bytecode[0..size]);
+        defer c.luau_free(bytecode);

         if (c.luau_load(lua.state, "...", bytecode, size, 0) != 0) return error.Fail;
     }

IMO adding a wrapper for free() to match exactly what the underlying Luau would do is a safe bet here. Although on second read, it may make sense not to add this in Luau's code but rather add a Zigluau specific C++ file for the same.

@nurpax
Copy link
Contributor

nurpax commented Dec 31, 2023

I started working on an example that loads precompiled bytecode (e.g., use luau-compile --binary test.luau > test.bc, then luau_load that). I think I can make a PR for an example and whatever APIs are needed for it.

However, a question on types. To load bytecode, I'm planning to add something like this:

    /// Loads Luau bytecode
    /// See https://luau-lang.org/getting-started
    pub fn loadBytecode(lua: *Lua, chunkname: [:0]const u8, bytecode: []const u8) !void {
        if (c.luau_load(lua.state, chunkname.ptr, bytecode.ptr, bytecode.len, 0) != 0) return error.Fail;
    }

Does the use of non-sentinel bytecode slice raise eyebrows here? IMO those are always a lot more convenient to use in Zig and there's no real reason not to use a normal slice here (since Luau accepts a ptr and length). But it's not consistent with the rest of the Zig Lua API.

Otherwise, I have a small example that's able to load a bytecode file and execute the loaded module. This stuff is exceptionally poorly documented in Luau, so I'll play with this a bit more to see that I did everything right. For example, I have no idea what the above chunkname means -- it seems like I can give it any value and it still works.

@natecraddock
Copy link
Owner Author

natecraddock commented Dec 31, 2023

OK, here's a patch that fixes the error on Windows. AFAICT the problem is that it's not portable to assume that std.heap.c_allocator.free() is the same as C lib's free(). It makes sense looking at the implementation of the c_allocator in Zig

Thanks for catching this! I did just assume that c_allocator was the same as libc. I'll take a look into fixing this based on your patch. This is the first time something in Ziglua has differed on Windows. This is a good motivation to add Windows coverage in the automated tests.

I started working on an example that loads precompiled bytecode (e.g., use luau-compile --binary test.luau > test.bc, then luau_load that). I think I can make a PR for an example and whatever APIs are needed for it.

Does the use of non-sentinel bytecode slice raise eyebrows here? IMO those are always a lot more convenient to use in Zig and there's no real reason not to use a normal slice here (since Luau accepts a ptr and length). But it's not consistent with the rest of the Zig Lua API.

I very likely use sentinel slices way too much in Ziglua. For return types from Lua it makes more sense, but for passing data in to Lua I expect it is not needed as much as I have done it. Feel free to not use a sentinel in your PR.

This stuff is exceptionally poorly documented in Luau

I'm glad I'm not alone in feeling this 😂 I've had to read through the source code to determine how many of the API functions have changed.

I do actually know what chunkname is. Or at least a good guess. I think that is the name given to the loaded Lua chunk. So if the Lua vm ever needed to refer to the chunk (like reporting an error) that chunkname would be used. I haven't looked at the code in this case, but I'm pretty confident that is what it is.

Please feel free to give more suggestions on the Luau API. I know you said you aren't super familiar with it, but based on things you said (like "I think the common approach to using Luau is to compile luau code into bytecode and then load bytecode only.") you appear to be more familiar than me.

Thanks!

@nurpax
Copy link
Contributor

nurpax commented Jan 1, 2024

not portable to assume that std.heap.c_allocator.free() is the same as C lib's free().
This is the first time something in Ziglua has differed on Windows

Clarifying my earlier comment: I'd say it's not a portability assumption even, just that one shouldn't assume a particular implementation for the c_allocator. Changes to c_allocator might well break code depending on c_allocator.free() == free() on Unix platforms too.

For pure malloc/free, there's also: https://ziglang.org/documentation/master/std/#A;std:heap.raw_c_allocator. But I think it's better done as in my patch so that whatever gets allocated in C is also deallocated in C.

Please feel free to give more suggestions on the Luau API

Sure! I'll need to use it for something real to see what's missing.

Chunkname: Yep, I think you're right. I found out later that it's a concept in "core Lua" too, so pretty sure that's what it means here too.

sentinel slices way [...] For return types from Lua it makes more sense

Yeah, it's easy to go from a sentinel slice to normal slice, so a sentinel return type is fine (and preferred if it means one less allocation) but the other direction is harder as it implies an allocation (need one more byte for the ending 0). I thought there was a function for converting from slice to zero-terminated string but I can't even find it now in the std lib. :)

@natecraddock natecraddock linked a pull request Jan 2, 2024 that will close this issue
@natecraddock
Copy link
Owner Author

I pushed some fixes, added windows CI coverage, and made a PR (probably should have done that a while ago at this point). Looks like everything is good to go. I'll probably merge this PR within the next day because at this point any future fixes or additional features can be done later.

#36

@nurpax
Copy link
Contributor

nurpax commented Jan 2, 2024

Yeah, I think it makes sense to merge. Easier to add stuff on top of it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants