From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 03770469719 for ; Fri, 6 Mar 2020 19:33:57 +0300 (MSK) Date: Fri, 6 Mar 2020 19:28:36 +0300 From: Igor Munkin Message-ID: <20200306162836.GC404@tarantool.org> References: <6b8602a297876492179b695f317e809bd425af79.1582209853.git.imun@tarantool.org> <79f7fafa-0d2f-ee26-f5f3-c6766b86d056@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <79f7fafa-0d2f-ee26-f5f3-c6766b86d056@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for your review! I sent the second version for the patch. On 03.03.20, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 2 comments below. > > > build: disable LUAJIT_ENABLE_PAIRSMM > > > > Since this build flag has been removed as a result of reverting the > > tarantool/luajit@d4e985a, its definition in the corresponding Tarantool > > cmake file is irrelevant. > > > > Furthermore, considering the breakage faced in #4770 the following tests > > are introduced: > > * the check whether space __pairs metamethod is set to space.pairs to > > create a Lua Fun iterator that handles __pairs manually underneath. > > * the check whether pairs builtin behaviour doesn't change when __pairs > > is set e.g. on space object. > > > > Follow-up #4560 > > Closes #4770 > > > > Signed-off-by: Igor Munkin > > > > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake > > index 072db8269..10df633d5 100644 > > --- a/cmake/luajit.cmake > > +++ b/cmake/luajit.cmake > > @@ -217,7 +217,6 @@ macro(luajit_build) > > add_definitions(-DLUAJIT_USE_ASAN=1) > > set (luajit_ldflags ${luajit_ldflags} -fsanitize=address) > > endif() > > - add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1) > > add_definitions(-DLUAJIT_SMART_STRINGS=1) > > # Add all COMPILE_DEFINITIONS to xcflags > > get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS) > > diff --git a/test/box/misc.result b/test/box/misc.result > > index 5ac5e0f26..6fc035171 100644 > > --- a/test/box/misc.result > > +++ b/test/box/misc.result > > @@ -1431,3 +1431,43 @@ test_run:grep_log('default', 'test log') > > --- > > - test log > > ... > > +-- > > +-- gh-4770: Iteration through space with Lua builtin pairs routine > > +-- > > +box.cfg{} > > 1. box.cfg{} is already done by test/box/box.lua, which starts > misc.test.lua. > Since the test is moved to a separate file (as was requested by Kirill), I left the box.cfg call. > > +--- > > +... > > +s = box.schema.create_space('test') > > +--- > > +... > > +-- Check whether __pairs is set for the space object, since Lua Fun > > +-- handles it manually underneath. > > +getmetatable(s).__pairs == space.pairs > > +--- > > +- true > > +... > > +-- Check whether pairs builtin behaviour doesn't change when the > > +-- __pairs is set. > > +keys = {} > > +--- > > +... > > +for k, v in pairs(s) do keys[k] = true end > > +--- > > +... > > +keys > > +--- > > +- engine: true > > + before_replace: true > > + field_count: true > > + id: true > > + on_replace: true > > + temporary: true > > + index: true > > + is_local: true > > + enabled: true > > + name: true > > + ck_constraint: true > > 2. I wouldn't print all the keys, since this test will break > anytime when something added to space. I would just check that > a couple of fields exists: keys.engine, keys.id, keys.name. > Up to you. IMHO this as a minor change and I guess we can postpone this remark waiting the second reviewer's (I hope Sasha will take a look soon) opinion. Ignoring for now. > -- Best regards, IM