lua: make more compatible with lua 5.3 #1914
Conversation
Lua 5.3 introduced stricter integer typisation which leaded to some errors in osc modules. pushnumber was changed to pushinteger where applicable, math.floor was used to forcibly convert floats to ints, an explicit typecast to lua_Integer was introduced in places where the error might occure with lua compiled with 32bit ints, and finally, one special corner case in get_screen_size which leaded to division by zero was taken care of.
|
Looks mostly fine, but there are problems. In a lot of places you slice off 64 bit integers. While Lua 5.1/2 will preserve about 52 bits of the 64 bit integers, your patch cuts them down to 32 bit (at least on 32 bit architectures, where lua_Integer is 32 bits for some reason). Also, I have doubt you caught all places. (It's not easy because the whole language suddenly changed.) But most severely, osc.lua seems to require changes. I'm not sure how much of it needs to be changed due to mpv Lua API behaving differently, and how much due to the language changing itself. Who is going to fix all the userscripts that have been written so far? Compiling mpv with Lua 5.3 instead of 5.1/2 would mean breaking them all. This is unreasonable. All in all, I don't think this is worth the trouble just because one distro thinks everyone should now switch to Lua 5.3. I've also just noticed that someone else already tried this in #1516 (with similar problems). |
| @@ -527,7 +533,10 @@ static int script_wait_event(lua_State *L) | |||
| pushnode(L, prop->data); | |||
| break; | |||
| case MPV_FORMAT_DOUBLE: | |||
| lua_pushnumber(L, *(double *)prop->data); | |||
| if (is_int(*(double *)prop->data)) | |||
| lua_pushinteger(L, *(lua_Integer *)prop->data); | |||
There was a problem hiding this comment.
This makes no sense. You can't derefence a double* as luaInteger*.
|
About cutting down ints to 32 bits, yes it's bad, but actually, when we are talking about IDs, I don't see how dropping 12 bits is better than dropping 32. Even dropping 1 bit will screw the things up. Otherwise, the patch doesn't really introduce any 5.3-specific fixes. A strict typisation is always a good thing, and the fact that lua doesn't make a difference between int and double doesn't mean that you should use floats everywhere without any thoughts. As for the corner case in get_screen_size, I think division by zero IS an error and should be fixed, even if lua 5.2 doesn't show it as such. About osc.lua: I looked into it. It seemed fine. There WERE problems, like when you try to change the track it would convert "1" to "1.0" and hence fail, but this patch takes care of it. |
No it's not. Why would you think that? |
Because it happens in window size calculation. Window sizes can't be inf's, it's illogical. |
ghost
|
This has been stalled for more than a year, no reason to keep it open. It can be reopened in the future if we decide to move to Lua 5.3. |
|
Or can you perhaps merge it to some additional branch? I was actually thinking about removing my mpv repo. |
|
@radistmorse merge both commits, git format-patch, and post it here as a comment (as the changes aren't huge). Then will be able to delete your fork |
ghost
mentioned this pull request
Lua 5.3 introduced stricter integer typisation which leaded to some
errors in osc modules. pushnumber was changed to pushinteger where
applicable, math.floor was used to forcibly convert floats to ints,
an explicit typecast to lua_Integer was introduced in places where
the error might occure with lua compiled with 32bit ints, and
finally, one special corner case in get_screen_size which leaded
to division by zero was taken care of.
This solves the "OSD not showing" bug with lua 5.3