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

Add lua-5.4 to list of libluas to consider #1118

Closed
wants to merge 1 commit into from
Closed

Add lua-5.4 to list of libluas to consider #1118

wants to merge 1 commit into from

Conversation

donaldcallen
Copy link
Contributor

modified: configure
Cast SIZE_MAX to lua_number before comparing it to n (a variable of that type) to avoid compile-time warning.
modified: vis-lua.c

modified:   configure
Cast SIZE_MAX to lua_number before comparing it to n (a variable of that type) to avoid compile-time warning.
modified:   vis-lua.c
@donaldcallen donaldcallen changed the title AAdd lua-5.4 to list of libluas to consider Add lua-5.4 to list of libluas to consider Aug 24, 2023
Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Don,

Thanks for the patch! If possible could you split this into two separate
commits. The configure change is fine and can be applied but I'm not
sure about the second one, please see inline.

@@ -496,7 +496,7 @@ static size_t getpos(lua_State *L, int narg) {

static size_t checkpos(lua_State *L, int narg) {
lua_Number n = luaL_checknumber(L, narg);
if (n >= 0 && n <= SIZE_MAX && n == (size_t)n)
if (n >= 0 && n <= (lua_Number) SIZE_MAX && n == (size_t)n)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has come up before but I can't remember where. I'm just not sure this
is correct. The problem, as the compiler warns about, is that the value
of SIZE_MAX changes when it is implicitly cast to lua_Number. The
value it changes to is dependent on the version of lua that vis is being
built against. The goal is to make sure that n falls in the range
[0, SIZE_MAX]. This shouldn't depend on the lua version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure your doubts about this code are right, with or without my cast (which just papers over the problem by eliminating the compiler warning).

Consider 64-bit machines. The issue is that SIZE_MAX is 0xFFFFFFFFFFFFFFFF on 64-bit machines, obviously requiring 64 bits to represent precisely. The default representation for lua_Number is double on 64-bit machines, which have 53 bits of precision. So SIZE_MAX can't be represented precisely as a double; it gets rounded up to the next higher integer that can be represented precisely as a double, namely 0x10000000000000000, which only requires 1 bit of significand precision as a double. The next such integer is 0x10000000000000000+0x800. 0x800 = 2048 = 2^(64-53). So when you get above (2^53)-1, things start to get imprecise and the kind of comparison the code does gets less and less reliable.

I think the solution to this is to check that n <=min(SIZE_MAX, (2^)-1) for whatever word size machine you are dealing with. This reduces the number of positions the editor can handle. Not a problem on 64-bit machines, where Lua uses doubles by default, so the limit is still astronomical. It could be a problem on 32-bit machines, but solvable where necessary by configuring Lua to use doubles, not floats.

I'll split the pull request into two tomorrow, as you requested.

@donaldcallen donaldcallen closed this by deleting the head repository Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants