[Tarantool-patches] [PATCH v2 1/2] lua: introduce table.equals method
Бабин Олег
olegrok at tarantool.org
Mon Aug 16 18:41:40 MSK 2021
Ok. Thanks for your changes. Don’t forget to changelog entry and docbot request and LGTM then.
>Понедельник, 16 августа 2021, 18:36 +03:00 от Serge Petrenko <sergepetrenko at tarantool.org>:
>
>
>
>16.08.2021 16:03, Бабин Олег пишет:
>> Thanks for your fixes.
>>
>> Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko
>> < sergepetrenko at tarantool.org >:
>>
>> Thanks for a thorough review!
>>
>> I've force pushed the following fixes:
>>
>> =============================================
>>
>> diff --git a/src/lua/table.lua b/src/lua/table.lua
>> index 3d5e69e97..edd60d1be 100644
>> --- a/src/lua/table.lua
>> +++ b/src/lua/table.lua
>> @@ -65,8 +65,11 @@ local function table_equals(a, b)
>> if type(a) ~= 'table' or type(b) ~= 'table' then
>> return type(a) == type(b) and a == b
>> end
>> - local mt = getmetatable(a)
>> - if mt and mt.__eq then
>> + local mta = getmetatable(a)
>> + local mtb = getmetatable(b)
>> + -- Let Lua decide what should happen when at least one of the
>> tables has a
>> + -- metatable.
>> + if mta and mta.__eq or mtb and mtb.__eq then
>>
>> I’m not sure that it’s correct. I’ve added a reference to SO
>> ( https://stackoverflow.com/questions/61946190/lua-eq-on-tables-with-different-metatables
>> < https://stackoverflow.com/questions/61946190/lua-eq-on-tables-with-different-metatables >)
>> and it seems condition should be following
>> mta and mta.__eq and mta == mtb. Otherwise tables have different __eq
>> metamathods and you should compare their content.
>> I’m not sure that my proposal is completely correct and suggest to ask
>> Igor about it.
>
>Actually the question you refer to describes Lua 5.3, while LuaJIT
>supports Lua 5.1
>The rules for Lua 5.1 are as follows (taken from
>https://www.lua.org/manual/5.1/manual.html#2.4 ):
>
>Use the __eq metamethod only when both objects have the same one.
>
>So, in my case, if at least one metamethod is defined, try to use
>comparison operator.
>If both tables have the same __eq metamethod, Lua will call it.
>If the tables don't share the same metamethod, they will be compared like
>Lua does usually: by reference. The result will be false then.
>
>
>I've added one more test though, just in case:
>
>diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
>index c7a075aba..ae6fdfa2d 100755
>--- a/test/app-tap/table.test.lua
>+++ b/test/app-tap/table.test.lua
>@@ -8,7 +8,7 @@ yaml.cfg{
> encode_invalid_as_nil = true,
> }
> local test = require('tap').test('table')
>-test:plan(43)
>+test:plan(44)
>
> do -- check basic table.copy (deepcopy)
> local example_table = {
>@@ -258,12 +258,15 @@ do -- check table.equals
> __eq = function(a, b) -- luacheck: no unused args
> return true
> end}
>- local tbl_d = setmetatable({a=15}, mt)
>- local tbl_e = setmetatable({b=2, c=3}, mt)
>+ local tbl_d = setmetatable({a = 15}, mt)
>+ local tbl_e = setmetatable({b = 2, c = 3}, mt)
> test:ok(table.equals(tbl_d, tbl_e), "table.equals respects __eq")
>- test:is(table.equals(tbl_d, {a=15}), false,
>+ test:is(table.equals(tbl_d, {a = 15}), false,
> "table.equals when metatables don't match")
>- test:is(table.equals({a=15}, tbl_d), false,
>+ test:is(table.equals({a = 15}, tbl_d), false,
>+ "table.equals when metatables don't match")
>+ local tbl_f = setmetatable({a = 15}, {__eq = function() return true
>end})
>+ test:is(table.equals(tbl_d, tbl_f), false,
> "table.equals when metatables don't match")
> end
>
>
>
>
>> return a == b
>> end
>> for k, v in pairs(a) do
>> diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua
>> index ec81593f3..029b923fb 100755
>> --- a/test/app-tap/table.test.lua
>> +++ b/test/app-tap/table.test.lua
>> @@ -8,7 +8,7 @@ yaml.cfg{
>> encode_invalid_as_nil = true,
>> }
>> local test = require('tap').test('table')
>> -test:plan(40)
>> +test:plan(43)
>>
>> do -- check basic table.copy (deepcopy)
>> local example_table = {
>> @@ -254,6 +254,14 @@ do -- check table.equals
>> test:ok(table.equals(tbl_a, tbl_c),
>> "table.equals for shallow copied tables after
>> modification")
>> test:is(table.equals(tbl_a, tbl_b), false, "table.equals does a
>> deep check")
>> + local mt = {__eq = function(a, b) return true end}
>> + local tbl_d = setmetatable({a=15}, mt)
>> + local tbl_e = setmetatable({b=2, c=3}, mt)
>> + test:ok(table.equals(tbl_d, tbl_e), "table.equals respects __eq")
>> + test:is(table.equals(tbl_d, {a=15}), false,
>> + "table.equals when metatables don't match")
>> + test:is(table.equals({a=15}, tbl_d), false,
>> + "table.equals when metatables don't match")
>> end
>>
>> os.exit(test:check() == true and 0 or 1)
>>
>> =============================================
>>
>> >
>> > On 13.08.2021 13:22, Serge Petrenko wrote:
>> >>
>> >>
>> >> 13.08.2021 08:30, Oleg Babin пишет:
>> >>> Thanks for your patch.
>> >>>
>> >>>
>> >>> It seems it works in quite strange way:
>> >>>
>> >>> ```
>> >>>
>> >>> tarantool> table.equals({a = box.NULL}, {})
>> >>> ---
>> >>> - true
>> >>> ...
>> >>>
>> >>> tarantool> table.equals({}, {a = box.NULL})
>> >>> ---
>> >>> - false
>> >>> ...
>> >>>
>> >>> ```
>> >>>
>> >>>
>> >>> I can change arguments order to get different result. Expected
>> false
>> >>> in both cases.
>> >>>
>> >>> For tap it was considered as buggy behaviour
>> >>> https://github.com/tarantool/tarantool/issues/4125
>> < https://github.com/tarantool/tarantool/issues/4125 >
>> >>>
>> >>>
>> >>
>> >> ====================================
>> >>
>> >> Good catch! Thanks!
>> >>
>> >> Check out the diff:
>> >>
>> >> diff --git a/src/lua/table.lua b/src/lua/table.lua
>> >> index 5f35a30f6..3d5e69e97 100644
>> >> --- a/src/lua/table.lua
>> >> +++ b/src/lua/table.lua
>> >> @@ -63,7 +63,7 @@ end
>> >> -- @return true when the two tables are equal (false otherwise).
>> >> local function table_equals(a, b)
>> >> if type(a) ~= 'table' or type(b) ~= 'table' then
>> >> - return a == b
>> >> + return type(a) == type(b) and a == b
>> >> end
>> >> local mt = getmetatable(a)
>> >> if mt and mt.__eq then
>> >> diff --git a/test/app-tap/table.test.lua
>> b/test/app-tap/table.test.lua
>> >> index a3c9aa123..ec81593f3 100755
>> >> --- a/test/app-tap/table.test.lua
>> >> +++ b/test/app-tap/table.test.lua
>> >> @@ -8,7 +8,7 @@ yaml.cfg{
>> >> encode_invalid_as_nil = true,
>> >> }
>> >> local test = require('tap').test('table')
>> >> -test:plan(38)
>> >> +test:plan(40)
>> >>
>> >> do -- check basic table.copy (deepcopy)
>> >> local example_table = {
>> >> @@ -227,6 +227,10 @@ do -- check table.equals
>> >> test:ok(table.equals({}, {}), "table.equals for empty tables")
>> >> test:is(table.equals({}, {1}), false, "table.equals with one
>> >> empty table")
>> >> test:is(table.equals({1}, {}), false, "table.equals with one
>> >> empty table")
>> >> + test:is(table.equals({key = box.NULL}, {key = nil}), false,
>> >> + "table.equals for box.NULL and nil")
>> >> + test:is(table.equals({key = nil}, {key = box.NULL}), false,
>> >> + "table.equals for box.NULL and nil")
>> >> local tbl_a = {
>> >> first = {
>> >> 1,
>> >>
>> >> ====================================
>> >>> On 13.08.2021 02:30, Serge Petrenko via Tarantool-patches wrote:
>> >>>> Introduce table.equals for comparing tables.
>> >>>> The method respects __eq metamethod, if provided.
>> >>>>
>> >>>> Needed-for #5894
>> >>>> ---
>> >>>>
>> >>
>>
>> --
>> Serge Petrenko
>>
>> --
>> Oleg Babin
>
>--
>Serge Petrenko
--
Oleg Babin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210816/f2e1f849/attachment.htm>
More information about the Tarantool-patches
mailing list