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