From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 8E3EE6EC40; Mon, 16 Aug 2021 19:47:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8E3EE6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629132430; bh=nKR0I+/qMUihYUrOPHn2ail/Vbv29CPTjyNlJmzYWz4=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=QapuhcKpvSh91Oy4glEvFA6O+eaMoiaJY1otrmubS0qTkveQwJmlVYMOm2qjl108r shXs/i8gjBCuaq6FSmD+oesyqkaJm6EjQFlv/dd5poCaG11rmaci1fy42eDEZqH6CD YyyPz8H8r/koGnxuCPpp9ulMEBgIt1FWkDblGwAQ= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BF58F6EC40 for ; Mon, 16 Aug 2021 19:47:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF58F6EC40 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1mFflT-0007Ot-KK; Mon, 16 Aug 2021 19:47:08 +0300 To: =?UTF-8?B?0JHQsNCx0LjQvSDQntC70LXQsw==?= References: <1629119031.480511653@f471.i.mail.ru> <0aac4487-0928-e3ab-3ef4-8140e6cf3566@tarantool.org> <1629128500.838902794@f338.i.mail.ru> Message-ID: Date: Mon, 16 Aug 2021 19:47:07 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <1629128500.838902794@f338.i.mail.ru> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9BCE6B93DE0C6C3914462CDB1732D383C182A05F538085040B16D8AFA7C6A18CA08A2E963764E3C8166A3A380C7F0478D3661AB322A1429E8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE751C3618ECA219898EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637838376990962B7D5EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2D65906AED435101420D16F8460965AB2CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F3E38EE449E3E2AE389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7356D8C47C27EEC5E9FB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5351488E732918D8C3DB6F89AEE89FADB9E6C92CDDDD96D55D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7567C209D01CC1E34B410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340A4C04F5DECA7EE95FFBD87E49477C29783F576589AD522ECEC66951B15165FA4FE33E21404086F61D7E09C32AA3244C8836682B2CBCB21BD6F6E94793E105CE55E75C8D0ED9F6EE927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojIrFL/N5KnVFP/LbDaqvhiQ== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446F44EC787237800EF01098B6C8DBBD89BD34B31F94090839A424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 1/2] lua: introduce table.equals method X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 16.08.2021 18:41, Бабин Олег пишет: > Ok. Thanks for your changes. Don’t forget to changelog entry and > docbot request and LGTM then. > Thanks for your answers! Yep, sure. Done. > Понедельник, 16 августа 2021, 18:36 +03:00 от Serge Petrenko > : > > > 16.08.2021 16:03, Бабин Олег пишет: > > Thanks for your fixes. > > > > Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko > > >: > > > > 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 > > > > >) > > 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 > > > > > > >>> > > >>> > > >> > > >> ==================================== > > >> > > >> 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 -- Serge Petrenko