From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104])
 (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 DB1A7469719
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  4 Mar 2020 01:53:52 +0300 (MSK)
References: <6b8602a297876492179b695f317e809bd425af79.1582209853.git.imun@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <79f7fafa-0d2f-ee26-f5f3-c6766b86d056@tarantool.org>
Date: Tue, 3 Mar 2020 23:53:51 +0100
MIME-Version: 1.0
In-Reply-To: <6b8602a297876492179b695f317e809bd425af79.1582209853.git.imun@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Igor Munkin <imun@tarantool.org>, Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

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 <imun@tarantool.org>
> 
> 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.

> +---
> +...
> +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.