Repository navigation
Fix string handling in pushAny - #231
AshAmetrine wants to merge 1 commit into
Conversation
robbielyman
left a comment
robbielyman
left a comment
There was a problem hiding this comment.
Some of these changes are sort of surprising to me!
| std.debug.assert(childinfo.child == u8); | ||
| std.debug.assert(childinfo.sentinel() != null); | ||
|
|
||
| if (childinfo.sentinel()) |sentinel| { |
There was a problem hiding this comment.
why is this code deleted?
There was a problem hiding this comment.
This assertion rejected pointers to arrays without a sentinel, such as: *const [2]u8.
| } | ||
| _ = lua.pushStringZ(std.mem.span(value)); | ||
| }, | ||
| .c => @compileError("C pointer length cannot be determined; pass a slice instead"), |
There was a problem hiding this comment.
why is this here? Lua itself obviously works with C pointers all the time?
There was a problem hiding this comment.
Quoting the Zig documentation: "The only valid reason for using a C pointer is in auto-generated code from translating C code."
I see no reason why they should be supported. The caller should know the type.
There was a problem hiding this comment.
It is possible that the value comes from another C library. I have recently been doing work using Lua and sqlite and I need to work with C strings occasionally. I think C pointers should be supported
There was a problem hiding this comment.
In that case, you would either know the length or whether it has a null terminator, then you could use either ptr[0..len] or std.mem.span(ptr).
If we supported C pointers with pushAny, it would be making the assumption that it has a null terminator, which results in UB when there isn't one.
Side note: I did not remove support for C pointers in this change. I only changed the error message.
| } | ||
|
|
||
| /// Pushes any string type | ||
| fn pushAnyString(lua: *Lua, value: anytype) error{OutOfMemory}!void { |
There was a problem hiding this comment.
is there really no error condition?
There was a problem hiding this comment.
Not since the unnecessary temporary allocation was removed.
This branch has not been deployed
3 participants
Changes:
[*:0]const u8)pushAnytest to cover support for[*:0]const u8and embedded null bytes in string literals