[Tarantool-patches] [PATCH v2 1/2] lua: introduce table.equals method
Serge Petrenko
sergepetrenko at tarantool.org
Mon Aug 16 19:47:07 MSK 2021
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
> <sergepetrenko at tarantool.org>:
>
>
> 16.08.2021 16:03, Бабин Олег пишет:
> > Thanks for your fixes.
> >
> > Понедельник, 16 августа 2021, 10:53 +03:00 от Serge Petrenko
> > <sergepetrenko at tarantool.org
> </compose?To=sergepetrenko at 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>
> >
> <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
> <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>
> > <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
>
> --
> Oleg Babin
--
Serge Petrenko
More information about the Tarantool-patches
mailing list