Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Бабин Олег" <olegrok@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 18:36:10 +0300	[thread overview]
Message-ID: <0aac4487-0928-e3ab-3ef4-8140e6cf3566@tarantool.org> (raw)
In-Reply-To: <1629119031.480511653@f471.i.mail.ru>



16.08.2021 16:03, Бабин Олег пишет:
> 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 
> <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


  reply	other threads:[~2021-08-16 15:36 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
2021-08-16 15:36             ` Serge Petrenko via Tarantool-patches [this message]
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=0aac4487-0928-e3ab-3ef4-8140e6cf3566@tarantool.org \
    --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