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

Use lua_Integer instead of lua_Number for checksum calculation #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jbsgh
Copy link

@jbsgh jbsgh commented Mar 5, 2021

We are using Lua on an embedded system with 32 bit. Accordingly, lua_Numbers are 32 bit floats not able to store 32 bit integer-checksums. I've replaced all calls like lua_pushnumber(), lua_tonumber() etc. with their integer counterparts like lua_pushinteger(), lua_tointeger etc. Besides I've added two checks in test.lua to show the correct results.

We use Lua 5.3 and 5.4 only. I didn't check for compatibility issues with older Lua versions that might not support integers correctly. If you merge this PR you may have to add conditions like #if (LUA_VERSION_NUM >= 503) to maintain compatibility with older versions.

@brimworks
Copy link
Owner

brimworks commented Mar 5, 2021

@hishamhm what do you think of dropping support for Lua 5.2?

@jbsgh thanks for the contribution. Let me think about this a bit before I merge it.

@brimworks
Copy link
Owner

Overall, this looks good, but I'd like to preserve lua 5.2 and luajit support at a minimum. If you can update your PR so that the proper #ifdefs are in place such that it works for these older version of Lua then I'll accept it.

Thanks!

@hishamhm
Copy link
Contributor

hishamhm commented Mar 9, 2023

Lua 5.1 does have lua_tointeger and lua_pushinteger. It just doesn't have lua_isinteger... Maybe that could be changed back to plain lua_isnumber in this PR (that looks like just a sanity check) and then we'd have support for all Lua versions without ifdefs?

@jbsgh
Copy link
Author

jbsgh commented Mar 10, 2023

lua_isnumber will not work correctly with 32-bit operations, as I stated in my OP.

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.

None yet

3 participants