-
Notifications
You must be signed in to change notification settings - Fork 3
Support bitwise operations in luaL_loadbuffer_proto #137
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
base: master
Are you sure you want to change the base?
Conversation
e1d1dde
to
85d173c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Sergey!
Thanks for the patch-set!
I'll proceed with the review per-patch below.
[PATCH 1/3] extra: add more errors found in PUC Rio Lua
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[PATCH 2/3] tests/capi: support floor division operation
Thanks for the patch!
Please consider my comment below.
@@ -70,11 +70,15 @@ end | |||
local __unm = function(v) | |||
return - always_number(v) | |||
end | |||
local __idiv = function(v1, v2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break parsing for Lua versions below 5.3:
luajit: /tmp/t5.3.lua:2: unexpected symbol near '/'
It is possible to workaround it via loadstring()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed:
diff --git a/tests/capi/luaL_loadbuffer_proto/preamble.lua b/tests/capi/luaL_loadbuffer_proto/preamble.lua
index 06912a0..edb56a8 100644
--- a/tests/capi/luaL_loadbuffer_proto/preamble.lua
+++ b/tests/capi/luaL_loadbuffer_proto/preamble.lua
@@ -91,7 +91,10 @@ local __unm = function(v)
return - always_number(v)
end
local __idiv = function(v1, v2)
- return always_number(v1) // always_number(v2)
+ -- `//` breaks parsing in PUC Rio Lua 5.1 and 5.1:
+ -- `unexpected symbol near '/'`.
+ local chunk = ('%d // %d'):format(always_number(v1), always_number(v2))
+ return assert(loadstring(chunk))()
end
local __band = function(v1, v2)
local band = bitwise_op('&')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadstring()
is undefined for the Lua 5.4:
lua5.4 -e 'loadstring""'
lua5.4: (command line):1: attempt to call a nil value (global 'loadstring')
stack traceback:
(command line):1: in main chunk
[C]: in ?
Plus, IMO, It is better to do something like the following:
_G.always_number = always_number
local __idiv = load[[
local v1, v2 = ...
return always_number(v1) // always_number(v2)
]]
In this key this metamethod will be uninitialized for the incorrect syntax cases.
Also, this helps to avoid calls to load
on every invokation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[PATCH 3/3] tests/capi: support bitwise operations in luaL_loadbuffer_proto
Thanks for the patch!
Please consider my comment below.
@@ -73,6 +73,24 @@ end | |||
local __idiv = function(v1, v2) | |||
return always_number(v1) // always_number(v2) | |||
end | |||
local __band = function(v1, v2) | |||
return always_number(v1) & always_number(v2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break parsing for Lua versions below 5.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed:
diff --git a/tests/capi/luaL_loadbuffer_proto/preamble.lua b/tests/capi/luaL_loadbuffer_proto/preamble.lua
index eea7336..edb56a8 100644
--- a/tests/capi/luaL_loadbuffer_proto/preamble.lua
+++ b/tests/capi/luaL_loadbuffer_proto/preamble.lua
@@ -8,6 +8,26 @@ local not_nan_and_nil = function(val)
return (val ~= val or val == nil) and DEFAULT_NUMBER or val
end
+local function bitwise_op(op_name)
+ return function(...)
+ local n = select('#', ...)
+ local chunk
+ -- Bitwise exclusive OR and bitwise NOT have the same
+ -- operator.
+ if (op_name == '&' or op_name == '|') then
+ assert(n > 1)
+ end
+ if n == 1 then
+ local x = ...
+ chunk = ('return %s %d'):format(op_name, x)
+ else
+ local op_name_ws = (' %s '):format(op_name)
+ chunk = 'return ' .. table.concat({...}, op_name_ws)
+ end
+ return assert(load(chunk))()
+ end
+end
+
local __add = function(v1, v2)
return always_number(v1) + always_number(v2)
end
@@ -77,22 +97,28 @@ local __idiv = function(v1, v2)
return assert(loadstring(chunk))()
end
local __band = function(v1, v2)
- return always_number(v1) & always_number(v2)
+ local band = bitwise_op('&')
+ return band(always_number(v1), always_number(v2))
end
local __bor = function(v1, v2)
- return always_number(v1) | always_number(v2)
+ local bor = bitwise_op('|')
+ return bor(always_number(v1), always_number(v2))
end
local __bxor = function(v1, v2)
- return always_number(v1) ~ always_number(v2)
+ local bxor = bitwise_op('~')
+ return bxor(always_number(v1), always_number(v2))
end
local __bnot = function(v)
- return ~always_number(v)
+ local bnot = bitwise_op('~')
+ return bnot(always_number(v))
end
local __shl = function(v1, v2)
- return always_number(v1) << always_number(v2)
+ local shl = bitwise_op('<<')
+ return shl(always_number(v1), always_number(v2))
end
local __shr = function(v1, v2)
- return always_number(v1) >> always_number(v2)
+ local shr = bitwise_op('>>')
+ return shr(always_number(v1), always_number(v2))
end
debug.setmetatable('string', {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using the same approach as I mentioned for the //
above.
Also, the n
value is always either 1 or 2.
The PUC Rio Lua 5.3 has introduced a floor division (//) operation. It is a division that rounds the quotient towards minus infinity, that is, the floor of the division of its operands. The patch adds support of the aforementioned operation to a protobuf schema, serializer and preamble.
The `bit32` library has been deprecated in PUC Rio Lua 5.3 [1]. Missed bitwise functions have been replaced by appropriate bitwise operations [2]. The patch adds support of bitwise operations to the test `luaL_loadbuffer_proto`. Bitwise operators introduced in Lua 5.3 breaks parsing in Lua 5.1 and 5.2, to avoid this metamethods in preamble.lua uses a function `bitwise_op`. It was implemented for bitwise tests added in the commit d4b91f6 ("tests/lapi: add bitop tests"). 1. https://www.lua.org/manual/5.3/manual.html#8.2 2. https://www.lua.org/manual/5.3/manual.html#3.4.2
85d173c
to
c410a55
Compare
No description provided.