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 512D46EC40; Mon, 16 Aug 2021 18:36:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 512D46EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629128174; bh=dttJZhUeRRG1LEDPhMVdIxV22ioxCGmpVtsMJ+xH5UI=; 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=PiCd6nUa/j/HTxG14ePgygBpDiYWHO17Gn3mrbmvoHrL7El1S18iDho/Gd18pKnXn ZCIevItNeur2lzchQfjtTFupk623WDNn9XyxYSM9Z5SqeNZbghr60lREZua8uFH90D mQtMKMtl3sHmKVfRqQN7dEVBaLJCKFbJFdQh6grY= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 A1DC66EC40 for ; Mon, 16 Aug 2021 18:36:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A1DC66EC40 Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1mFeep-0002Sa-8W; Mon, 16 Aug 2021 18:36:11 +0300 To: =?UTF-8?B?0JHQsNCx0LjQvSDQntC70LXQsw==?= References: <1629119031.480511653@f471.i.mail.ru> Message-ID: <0aac4487-0928-e3ab-3ef4-8140e6cf3566@tarantool.org> Date: Mon, 16 Aug 2021 18:36:10 +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: <1629119031.480511653@f471.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: 4F1203BC0FB41BD92087353F0EC44DD9BCE6B93DE0C6C3914462CDB1732D383C182A05F538085040937FFD400A8D026649058B80DBAF03ABC02E89812F31B8BEB9F408B7C54D12BC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E1F5C7CA1DBFB751EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063745476ED688D943148638F802B75D45FF143C0E33DA2437434630C57BCD80B8175B73F950BC6E7FFBB06A47469594D7D37F5F5716AA90537DA471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7850F8B975A76562C9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F616AD31D0D18CD5C57739F23D657EF2BB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5ADF8BE3DDB4D9C98CC0F609CC3DADE8B00357BEF14E3CD0CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346840168BCAD8054E50C84208EDE10F4A5290583054F7DA6687B11AB949BDD1A9CFED85FBA13985BC1D7E09C32AA3244C922A5B2F9DA2939D98C279BE3997150AE646F07CC2D4F3D8927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojIrFL/N5KnVHMKUqyrw+b/g== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446398F7559005E357B395C1E78739DE998D792504DEFD70700424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 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 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