Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Бабин Олег via Tarantool-patches" <tarantool-patches@dev.tarantool.org>
To: "Serge Petrenko" <sergepetrenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches]  [PATCH v2 1/2] lua: introduce table.equals method
Date: Mon, 16 Aug 2021 16:03:51 +0300	[thread overview]
Message-ID: <1629119031.480511653@f471.i.mail.ru> (raw)
In-Reply-To: <bec3b2c4-d2e4-69cc-f02c-bf4732d3fa65@tarantool.org>

[-- Attachment #1: Type: text/plain, Size: 5362 bytes --]


Thanks for your fixes.

  
>Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko <sergepetrenko@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 ) 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.
 
>          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
 

[-- Attachment #2: Type: text/html, Size: 8140 bytes --]

  reply	other threads:[~2021-08-16 13:03 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-12 23:30 [Tarantool-patches] [PATCH v2 0/2] allow upgrading from version 1.6 Serge Petrenko via Tarantool-patches
2021-08-12 23:30 ` [Tarantool-patches] [PATCH v2 1/2] lua: introduce table.equals method Serge Petrenko via Tarantool-patches
2021-08-13  5:30   ` Oleg Babin via Tarantool-patches
2021-08-13 10:22     ` Serge Petrenko via Tarantool-patches
2021-08-13 20:13       ` Oleg Babin via Tarantool-patches
2021-08-16  7:53         ` Serge Petrenko via Tarantool-patches
2021-08-16 13:03           ` Бабин Олег via Tarantool-patches [this message]
2021-08-16 15:36             ` Serge Petrenko via Tarantool-patches
2021-08-16 15:41               ` Бабин Олег via Tarantool-patches
2021-08-16 16:47                 ` Serge Petrenko via Tarantool-patches
2021-08-13 11:41   ` Vladimir Davydov via Tarantool-patches
2021-08-12 23:30 ` [Tarantool-patches] [PATCH v2 2/2] box: allow upgrading from version 1.6 Serge Petrenko via Tarantool-patches
2021-08-13 11:40   ` Vladimir Davydov via Tarantool-patches
2021-08-16 13:18   ` Бабин Олег via Tarantool-patches
2021-08-16 16:32     ` Serge Petrenko via Tarantool-patches
2021-08-16 18:22       ` Бабин Олег via Tarantool-patches
2021-08-14  8:12 ` [Tarantool-patches] [PATCH v2 0/2] " Vitaliia Ioffe via Tarantool-patches
2021-08-17  7:28 ` Kirill Yukhin via Tarantool-patches
2021-08-17  8:05   ` Kirill Yukhin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1629119031.480511653@f471.i.mail.ru \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --subject='Re: [Tarantool-patches]  [PATCH v2 1/2] lua: introduce table.equals method' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox