Skip to content

Conversation

moteus
Copy link

@moteus moteus commented May 23, 2014

I test on Windows XP x32.
Also I add Travis/Coveralls.io files for testing.

moteus added 4 commits May 23, 2014 15:25
The problem occurs when lua_Number is double and lua_Integer is int32
In this case `mp.unpack(mp.pack(0xFFFFFFFF)) == -1`
@mattsta
Copy link
Owner

mattsta commented May 27, 2014

Thanks! Great work getting all the CI stuff working too.

I've merged your commits and the travis results are showing up at https://travis-ci.org/mattsta/lua-cmsgpack

I did change your sizeof(lua_Integer) < 64 tests to be tests for == 4 instead. sizeof returns the size in bytes, so it looks like you were testing for "not a 64 bit type."

Feel free to re-run the tests on your 32 bit system and report back if I broke anything.

@mattsta mattsta closed this May 27, 2014
@moteus
Copy link
Author

moteus commented May 27, 2014

I did change your sizeof(lua_Integer) < 64 tests to be tests for == 4 instead.

oops. In fact prior Lua 5.3 there only lua_Number and we can use only it. But in this case we also should implement conversion from int64/int32 to lua_Number (C conversion is sufficient).

@moteus
Copy link
Author

moteus commented May 30, 2014

I think you lost this. :)

@mattsta
Copy link
Owner

mattsta commented May 30, 2014

Yeah, it kinda vanished.

The tests we added to check the fix actually pass before the commit on my system (and the CI system), so we need a better test to check for the problem.

The intended target environment for this package is 64 bit Unix systems, so maybe we should just maintain a different branch to satisfy 32 bit Windows requirements?

@moteus
Copy link
Author

moteus commented Jun 2, 2014

My usecase includes also Win2k3 x32. So this is critical for me.
I do not think is worth make two different modules.
There no big changes between them and it will be hard make deps in LuaRocks.
Ideally we need #if sizeof(lua_Integer) but this is not possible on C.

@moteus
Copy link
Author

moteus commented Jun 2, 2014

PS. This is also related to all platforms with sizeof(lua_Integer) < 8. I just test it with Linux Minx 15 x32.

mattsta added a commit that referenced this pull request Nov 24, 2014
This is the solution to #4 but rewritten
to use compile-time pointer size determination.
mattsta added a commit that referenced this pull request Nov 24, 2014
This is the solution to #4 but rewritten
to use compile-time pointer size determination.
mattsta added a commit that referenced this pull request Nov 24, 2014
This is the solution to #4 but rewritten
to use compile-time pointer size determination.
mattsta added a commit that referenced this pull request Nov 24, 2014
This is the solution to #4 but rewritten
to use compile-time pointer size determination.
@mattsta
Copy link
Owner

mattsta commented Nov 24, 2014

Finally got this added in 3f29140!

Thanks for the fix (even though it took a while)!

Also, I updated the TravisCI scripts to run from the latest 5.3.0-beta (instead of work2) and from luarocks 2.2 (instead of 2.1), but that broke other things in testing (see the two broken results at https://travis-ci.org/mattsta/lua-cmsgpack/builds/41987808). If you know how to fix those issues, go ahead :) I think one issue is the new luarocks doesn't remember the LUA_SFX properly because after you install luajit, luarocks goes back to just calling 'lua' and it fails.

@moteus
Copy link
Author

moteus commented Nov 24, 2014

  1. Lua 5.3 remove luaL_checkint (this is just alias for luaL_checkinteger). You can just use luaL_checkinteger (see lunarmodules/luafilesystem@8f29499).
  2. You can use travis files form lua-vararg repo. It works on all Lua versions and it does not use LUA_SFX.

If you want I can create PR tomorrow but it quite trivial.

mattsta added a commit that referenced this pull request Nov 25, 2014
This is the solution to #4 but rewritten
to use compile-time pointer size determination.
@mattsta
Copy link
Owner

mattsta commented Nov 25, 2014

Fixed! All tests are passing for all versions: https://travis-ci.org/mattsta/lua-cmsgpack

Thanks!

@moteus
Copy link
Author

moteus commented Nov 25, 2014

I tested on WinXP x86 with Lua 5.1/5.2/5.3.beta all tests pass.

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