pushInteger, checkInteger, pushNumber, checkNumber typing annoyances

[nurpax]

As Lua floats are f64 and as checkInteger returns c_longlong, I notice that code using ziglua ends up being a little annoying with @intCasts and @floatCasts:

    fn sheetSprite(lua: *Lua) i32 {
        const globals = lua_common.getZigObjects(lua);
        const self = lua_common.checkUserdataWithMeta(lua, sprites.Renderer, 1);
        const sheet_id: usize = @intCast(lua.checkInteger(2));
        const x = lua.checkNumber(3);
        const y = lua.checkNumber(4);
        const frame_idx: usize = @intCast(lua.checkInteger(5) - 1);

        const sheet: *const sprites.SpriteSheet = &globals.sprite_sheets.items[sheet_id];
        const image_id = sheet.image_id;
        const frame = sheet.frames[frame_idx % sheet.frames.len];

        const src_x: f32 = frame.x;
        const src_y: f32 = frame.y;
        const src_xy = Vec2.T{ src_x, src_y };
        const src_wh = Vec2.T{ frame.w, frame.h };
        self.sprite(.{ @floatCast(x), @floatCast(y) }, .{ frame.w, frame.h }, sprites.RenderMode{ .image = .{ .image_id = @intCast(image_id) } }, src_xy, src_wh);
        return 0;
    }

I wonder if it'd be a good idea to add something like this into ziglua (written in browser, didn't check if it compiles):

    pub fn checkNum(comptime T: type, lua: *Lua, arg: i32) Number {
        const lv = c.luaL_checknumber(@ptrCast(lua), arg)
        const v: T = @floatCast(lv);
        return v;
    }

And similarly for checkInteger and also for pushInteger, pushNumber.

I suppose I could add my own functions that do the same, but then the won't be usable as "methods" like lua.checkNumber(...) but rather myCheckNumber(lua, ...).

[robbielyman]

Do you mean to say something like

pub fn checkNumeric(lua: *Lua, comptime T: type, arg: i32) T {
    if (@typeInfo(T) == .integer) return @intCast(lua.checkInteger(arg));
    return @floatCast(lua.checkNumber(arg));
}

[nurpax]

Yes! That's better than what I had in mind.

I reviewed some of my binding code and this single feature would remove most of my @intCast/@floatCasts very nicely.

[robbielyman]

nice. i’d be happy to add it. one question i have is whether something other than @intCast should be used, since that builtin asserts in modes with runtime safety enabled?

[nurpax]

I had thought it should be @intCast. It will probably protect against some types of bugs, like converting from ints to u8 and noticing overflow. I'd imagine "some safety by default" is better than "truncate by default".

@floatCast is lossy anyway as going from f64 to f32 throws away bits without asserts.

I guess a more safe alternative would be to do the range checks in checkNumeric() for the given result type, and throw a real lua error if it's out of bounds. That's not what I had in mind in my original issue though.

My motivation was to get rid of the fairly frequent and annoying @intCasts that arise from my own types not aligning with what lua checkInt/checkInteger return. I'm interfacing with some ECS code where the values are compressed to different sizes like i32/u32, i16/u16, etc. Similarly, most of my floats are f32, so there's a @floatCast in practically every checkNumber call. I'm not looking for either automatic truncation or extra safety with range checks, just want the binding code to be more streamlined.

That's my reasoning.. but what did you have in mind instead of @intCast?

[robbielyman]

to be clear, you're saying "some safety by default is good" to mean "i want my program to crash if i called this function with bad Lua input". that's fine, and I think makes total sense. an alternative might be to use try std.math.cast instead of @intCast.

[nurpax]

Yes, I think I'd be mostly using the unsafe path with some expectation of input value ranges. I favor speed over safety in the game proto I'm working on. I'd expect these safety crashes to be fairly infrequent in my project, as the inputs should be pretty well behaved.

OTOH, someone building f.ex. a library probably can't make this kind of an assumption.

Should both be supported?

[robbielyman]

I think I'd like both of them, yeah—the safety-included version would certainly be useful to me in my Lua runtime project. I guess probably the "safe" versions of toNumeric and pushNumeric should return Zig errors, while check should throw a Lua error?

[nurpax]

I gave this some thought and looked at the casting code in the zig std library too. I've changed my mind a bit.. I think using std.math.cast is better than @intCast here. I think the perf cost of that checks is pretty trivial compared to how much other stuff runs as part of checkNumber.

Summarizing my thoughts:

• checkNumeric - use std.math.cast, throw a Lua error if the cast fails (ie., std.math.cast fail is treated the same as trying to convert a string to a number with this function)
• toNumeric - use std.math.cast, potentially return a Zig error just like toInteger does currently.
• pushNumeric - well, I had not thought about this one. Isn't the only case that doesn't type check really doing pushNumeric() with an u64 or usize? I think isize and any smaller than 64-bit ints will work here, and ditto for f32 and f64? It can be a bit of an annoyance that indices in Zig loops are usize, so pushing that will require an @intCast with pushInteger.

IMO checkNumeric is the utility that has a high $/! w.r.t reducing annoying casts whereas pushNumeric would exist for basically just one corner case of pushing an unsigned 64-bit int which cannot fit into i64.

[robbielyman]

I'm in agreement! I'll update the PR and ping you :)

[nurpax]

Great! :)

BTW I didn't say it explicitly but what I meant with the pushNumeric part is that that function is probably not necessary, since it's pretty well serviced by pushInteger, pushNumber.