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

try using native bitwise operations without checking lua version #177

Closed
wants to merge 2 commits into from

Conversation

jprjr
Copy link

@jprjr jprjr commented Nov 23, 2020

This is an alternative to #174

Rather than relying on _VERSION == "Lua 5.3", this just tries loading the Lua 5.3+ wrapper table no matter what. Assuming future versions of Lua keep bitwise ops, this should work on future versions of Lua.

Plus it should be compatible with forks/renames/etc.

@coveralls
Copy link

coveralls commented Nov 23, 2020

Coverage Status

Coverage increased (+0.009%) to 87.79% when pulling 702a126 on jprjr:future-lua into 47225d0 on daurnimator:master.

Copy link
Owner

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

Good idea :)

http/bit.lua Outdated
bor = function(a, b) return a | b end;
bxor = function(a, b) return a ~ b end;
}]], info.source))()
-- Lua 5.1's load function doesn't support reading strings, only
Copy link
Owner

Choose a reason for hiding this comment

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

Use loadstring in 5.1?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I guess it's six of one, half-dozen of the other.

This could be replaced with something like "local l = load; if loadstring then l = loadstring"

I can switch to something like that if you prefer, let me know!

Copy link
Owner

Choose a reason for hiding this comment

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

I guess loadstring doesn't have a source argument; so load is best.

It annoys me a little that we don't use straight up load in newer versions of lua.
I just realised we already depend on compat-5.3 in lua 5.1, so how about:

local load = load
if _VERSION == "Lua 5.1" then
    load = require "compat53.module".load
end

Copy link
Author

Choose a reason for hiding this comment

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

re: loadstring - in the docs they list the second parameter as chunk instead of source, but it's the same thing, loadstring should work and show the source filename as intended.

Copy link
Author

Choose a reason for hiding this comment

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

See latest push, I just did local load = loadstring or load

Copy link
Owner

Choose a reason for hiding this comment

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

I don't want to use loadstring if load is available.

@daurnimator
Copy link
Owner

See #180

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