[Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

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

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