From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 BA1FB469719 for ; Fri, 14 Feb 2020 01:50:19 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 13 Feb 2020 23:50:17 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/3] lua: table.deepcopy ignores __pairs metamethod List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: olegrok@tarantool.org, tarantool-patches@dev.tarantool.org, imun@tarantool.org Cc: Oleg Babin Thanks for the patch! See 5 comments below. 1. Commit title uses narration, while it should use imperative. For example, "lua: don't use pairs() in table.deepcopy". On 13/02/2020 21:33, olegrok@tarantool.org wrote: > From: Oleg Babin > > After 1d85144a9b4bbbb026402848efde1ab98bf72633 2. After commit hash please put its title in (). > table.deepcopy changed the behaviour and started > iterate through tables considering __pairs metamethod. > In some cases it broke backward compatibility. > To avoid such problem let's ignore __pairs > and iterate through tables as it was before > > Closes #4770 3. Unfortunately, things are not that simple. This patch does not really close the issue, because pairs(space_object) is still broken. But your commit probably is useful anyway in case we will decide to keep 5.2 __pairs/__ipairs. And it would unblock vshard development. Summary: just use 'Part of #4770' instead of 'Closes'. > Follow-up #4560 > --- > Issue: https://github.com/tarantool/tarantool/issues/4770 > Branch: https://github.com/tarantool/tarantool/tree/olegrok/table-fixes > src/lua/table.lua | 7 ++++++- > test/app-tap/table.test.lua | 26 +++++++++++++++++++++++++- > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/src/lua/table.lua b/src/lua/table.lua > index d83217dcb..4fa9c421c 100644 > --- a/src/lua/table.lua > +++ b/src/lua/table.lua > @@ -1,3 +1,8 @@ > +-- This pairs implementation doesn't trigger __pairs metamethod > +local function internal_pairs(tbl) > + return next, tbl, nil > +end 4. I would call it 'rawpairs', by analogue with 'rawset/get/*' functions. > + > local function table_deepcopy_internal(orig, cyclic) > cyclic = cyclic or {} > local copy = orig > @@ -10,7 +15,7 @@ local function table_deepcopy_internal(orig, cyclic) > copy = cyclic[orig] > else > cyclic[orig] = copy > - for orig_key, orig_value in pairs(orig) do > + for orig_key, orig_value in internal_pairs(orig) do > local key = table_deepcopy_internal(orig_key, cyclic) > copy[key] = table_deepcopy_internal(orig_value, cyclic) > end > diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua > index 07894f69e..3faf2ed23 100755 > --- a/test/app-tap/table.test.lua > +++ b/test/app-tap/table.test.lua > @@ -241,4 +241,28 @@ do -- gh-4340: deepcopy doesn't handle __metatable correctly. > ) > end > > +do -- gh-4770: deepcopy uses __pairs for iteration over table. > + local original = { a = 1, b = 2 } > + > + local function custom_pairs(self) > + local function step(tbl, k) > + local k, v = next(tbl, k) > + if v ~= nil then > + v = v + 1 > + end > + return k, v > + end > + return step, self, nil 5. I propose you to just add 'assert(false)' as a body of custom pairs. To ensure that it is never called by deepcopy. Because anyway it is not called now in your test. > + end > + > + setmetatable(original, {__pairs = custom_pairs }) > + > + -- Don't use is deeply as it could use pairs for check > + local copy = table.deepcopy(original) > + test:is(original.a, copy.a, > + "checking that the first values is correctly copied") > + test:is(original.b, copy.b, > + "checking that the second values is correctly copied") > +end > + > os.exit(test:check() == true and 0 or 1) >