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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ int main(int argc, char *argv[]) {
}
EOF

for liblua in lua lua5.4 lua5.3 lua5.2 lua-5.3 lua-5.2 lua54 lua53 lua52; do
for liblua in lua lua5.4 lua5.3 lua5.2 lua-5.4 lua-5.3 lua-5.2 lua54 lua53 lua52; do
printf " checking for %s... " "$liblua"

if test "$have_pkgconfig" = "yes" ; then
Expand Down
4 changes: 2 additions & 2 deletions vis-lua.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

return n;
return luaL_argerror(L, narg, "expected position, got number");
}
Expand Down Expand Up @@ -1343,7 +1343,7 @@ static int pipe_func(lua_State *L) {
}
const char *cmd = luaL_checkstring(L, cmd_idx);
bool fullscreen = lua_isboolean(L, cmd_idx + 1) && lua_toboolean(L, cmd_idx + 1);

if (!file)
return luaL_error(L, "vis:pipe(cmd = '%s'): win not open, file can't be nil", cmd);

Expand Down