Skip to content
This repository has been archived by the owner on Jan 4, 2021. It is now read-only.

Add lua 5.3 support #22

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

Add lua 5.3 support #22

wants to merge 6 commits into from

Conversation

daurnimator
Copy link

Solves #20

Notably this removes the int64 module
In 5.1 and 5.2, values with more than 53 bits of precision will be rounded.
However there has not been a release with any more than 32bits of precision (the int64 module has not been in any releases)

@daurnimator
Copy link
Author

Bignum test now fails in 5.1 and 5.2.
It passes in 5.3, but travis's ubuntu is so old it doesn't have 5.3 available.
so... I'm not sure what to do about the .travis.yml.

@golgote
Copy link
Member

golgote commented Mar 14, 2016

Some #ifdef might be needed for 5.1 (and 5.2 if int64 modules works with 5.2).
Pllua with Lua 5.1 should still be able to work with bigint IMO.
For Lua 5.3, I think it is possible to require that travis installs or compile something before tests are run, maybe just a wget on Lua 5.3 sources and make install.

@golgote
Copy link
Member

golgote commented Mar 14, 2016

Found some examples of travis.yml with Lua 5.3 and a few other versions here:
https://github.com/rtsisyk/luafun/blob/master/.travis.yml
https://github.com/awesomeWM/awesome/blob/master/.travis.yml

@daurnimator
Copy link
Author

Found some examples of travis.yml with Lua 5.3 and a few other versions here:
https://github.com/rtsisyk/luafun/blob/master/.travis.yml
https://github.com/awesomeWM/awesome/blob/master/.travis.yml

I now use 'hererocks' in all my lua projects. Changing that over belongs in a different PR to this one though.

@daurnimator
Copy link
Author

Some #ifdef might be needed for 5.1 (and 5.2 if int64 modules works with 5.2).
Pllua with Lua 5.1 should still be able to work with bigint IMO.

IMO the current int64 code is misled:

  • it's incompatible with other lua libraries
  • it doesn't mix with luajit's built-in 64bit integers
  • it's not forward compatible with 5.3

I'd like to just drop it: any reason not to should be grounded in deployed code that relies on the (unreleased) behaviour in git HEAD.

@eugwne
Copy link
Member

eugwne commented Mar 15, 2016

The main purpose of int64 - is using postgres bigint,
luajit 64 afaik is using boxing/unboxing for int/uint and a bit changed parser for nums like 2ULL - so not compatible to other lua too.
Lua 5.1 needs something to use bigint.
Luajit might use the same that works for 5.1 or migth be rewriten to use cdata implementation.
Lua 5.3 doesn't need this code.

I see that int64 code is unnecessary for lua5.3, but what about 5.1 and luajit?

@daurnimator
Copy link
Author

luajit 64 afaik is using boxing/unboxing for int/uint and a bit changed parser for nums like 2ULL - so not compatible to other lua too.

Correct, but adding a second int64 type when luajit already has one in the core seems incorrect to me.

Lua 5.1 needs something to use bigint.

Why not just recommend using lua 5.3 if you want to use bigint?

Luajit might use the same that works for 5.1 or migth be rewriten to use cdata implementation.

It's not easy to push a luajit int64_t.
I guess you could keep this function in a C upvalue:

local ffi = require "ffi"
local int64_t = ffi.typeof("int64_t")
return function(c)
    return ffi.cast(int64_t, c)[0]
end

and call it to push an int64_t.

@jmealo
Copy link

jmealo commented Nov 19, 2017

@daurnimator @eugwne: Kudos on the work. Where did we land on this?

@daurnimator
Copy link
Author

Unfortuneatly @mbalmer 's code was commited directly to master, and then @eugwne made a release :(

Someone will need to go through and clean out most of the #if conditional compilation added.

@daurnimator
Copy link
Author

I've pushed an updated branch.
Due to the release, I can no longer remove the int64 branch without a major version bump, so I've only added an extra 5.3 codepath.

We still don't have a codepath that takes advantage of luajit's numbers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants