From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 9C2884696C3 for ; Fri, 14 Feb 2020 01:50:20 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <699c3487-7fd4-9ca8-6de0-0bde4089d7ad@tarantool.org> Date: Thu, 13 Feb 2020 23:50:18 +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 1/3] lua: fix incorrect table.deepcopy __metatable handling 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 3 comments below. On 13/02/2020 21:33, olegrok@tarantool.org wrote: > From: Oleg Babin > > Before this patch we got metatabe of original table > using "getmetatable" function. It leads to the error > if original table contained __metatable that could be > e.g. a string or a number. 1. What error and why? (I know why, because I googled this, but it should be in the commit message). > To fix this problem getmetatable was replaced to debug.getmetatable. > > Closes #4340 > --- > Issue: https://github.com/tarantool/tarantool/issues/4340 > src/lua/table.lua | 2 +- > test/app-tap/table.test.lua | 20 +++++++++++++++++++- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/src/lua/table.lua b/src/lua/table.lua > index 7e8ba7aae..d83217dcb 100644 > --- a/src/lua/table.lua > +++ b/src/lua/table.lua > @@ -2,7 +2,7 @@ local function table_deepcopy_internal(orig, cyclic) > cyclic = cyclic or {} > local copy = orig > if type(orig) == 'table' then > - local mt, copy_function = getmetatable(orig), nil > + local mt, copy_function = debug.getmetatable(orig), nil 2. Please, add a comment why do you use debug instead of normal getmetatable(). > if mt then copy_function = mt.__copy end > if copy_function == nil then > copy = {} > diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua > index 60c095fdf..07894f69e 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(31) > +test:plan(33) > > do -- check basic table.copy (deepcopy) > local example_table = { > @@ -223,4 +223,22 @@ do -- check usage of not __copy metamethod on second level + shallow > ) > end > > +do -- gh-4340: deepcopy doesn't handle __metatable correctly. 3. Please, put comment on a separate line, before 'do'. The code above, which places comment and code on the same line, is incorrect. Probably because it is old, and at that time there was no such rule. > + local original = { > + content = 'string' > + } > + setmetatable(original, { __metatable = 'protection' }) > + local copy = table.deepcopy(original) > + test:is( > + copy.content, > + original.content, > + "checking that original string was copied" > + ) > + test:is( > + getmetatable(copy), > + 'protection', > + "checking that __metatable was correctly copied" > + ) > +end > + > os.exit(test:check() == true and 0 or 1) >