Re: [Tarantool-patches] [PATCH v2 luajit 20/30] test: adapt PUC Lua test for args in vararg func

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

Thanks for the review!

On 31.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! I can't understand why this patch is separated
> from the previous two (10 and 11). Could you provide a rationale for
> this, please? Also consider the comments below.

Answered about difference with 10th here [1].
I'm totally OK to squash it with 11th.

> 
> On 26.03.21, Sergey Kaplun wrote:
> > Lua 5.1 interprets ... in the vararg functions like additional first
> 
> Typo: s/like additional/as an additional/.

Fixed.

> 
> > argument, unlike LuaJIT does. All extra arguments is set into `arg`
> 
> Typo: s/arguments is set/arguments are set/.

Fixed.

> 
> > variable.
> > 
> > Implicit `arg` parameter for old-style vararg functions was finally
> > removed in Lua 5.2. This patch adjust tests in vararg.lua considering
> 
> Minor: Did LuaJIT always respect such behaviour? If no, please mention
> the commit where it has been changed.

Yes, at least since LuaJIT-2.0.0-beta1, AFAICS.
| $ src/luajit -v -e 'local function f(...) print(arg) end f(1, 3, 4)'
| LuaJIT 2.0.0-beta1 -- Copyright (C) 2005-2009 Mike Pall. http://luajit.org/
| nil

> 
> > Lua 5.2 test suite taken from
> > https://www.lua.org/tests/lua-5.2.0-tests.tar.gz.
> > 
> > Closes tarantool/tarantool#5712
> 
> As we discussed before: s/Closes/Resolves/.

Fixed.

> 
> > Part of tarantool/tarantool#5845
> > Part of tarantool/tarantool#4473
> > ---
> >  test/PUC-Lua-5.1-tests/vararg.lua | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/test/PUC-Lua-5.1-tests/vararg.lua b/test/PUC-Lua-5.1-tests/vararg.lua
> > index ae068fa..efb76c5 100644
> > --- a/test/PUC-Lua-5.1-tests/vararg.lua
> > +++ b/test/PUC-Lua-5.1-tests/vararg.lua
> > @@ -2,9 +2,13 @@ print('testing vararg')
> >  
> >  _G.arg = nil
> >  
> > +-- Lua 5.1 interprets ... in the vararg functions like additional
> 
> Typo: s/like additional/as an additional/.

Fixed.

> 
> > +-- first argument, unlike LuaJIT does. All extra arguments is set
> 
> Typo: s/arguments is set/arguments are set/.

Fixed.

> 
> > +-- into `arg` variable. This extension is from Lua 5.2.
> > +-- See also https://github.com/tarantool/tarantool/issues/5712.
> 
> Side note: What is the difference between #5712 and #5694? They look
> like duplicates to me, and so provide another rationale for squashing
> all three patches into a single one.

Answered about difference with 10th here [1]. Different tickets was
created to be able "to eat mamonth by parts" -- to fix later not all,
but single patch.
I'm totally OK to squash it with 11th.

> 
> > +-- LuaJIT: Test is adapted from PUC-Rio Lua 5.2 test suite.
> >  function f(a, ...)
> > -  assert(type(arg) == 'table')
> > -  assert(type(arg.n) == 'number')
> > +  local arg = {n = select('#', ...), ...}
> 
> Why did you drop the assertions above? They are trivial (as many of
> others in this suite), but still check that everything is fine (e.g.
> that <select> yields a number).

Just following Lua 5.2 test suite behaviour.
Side note: we do not test that select returns number here, and the
first assert is totally redundant.

> 
> >    for i=1,arg.n do assert(a[i]==arg[i]) end
> >    return arg.n
> >  end
> 
> <snipped>
> 
> > @@ -42,7 +48,9 @@ a = call(print, {'+'})
> >  assert(a == nil)
> >  
> >  local t = {1, 10}
> > -function t:f (...) return self[arg[1]]+arg.n end
> > +-- LuaJIT: Test chunk is adapted from PUC-Rio Lua 5.2 test suite.
> > +-- See the comment above.
> > +function t:f (...) local arg = {...}; return self[...]+#arg end
> 
> Why didn't you create the same structure as elsewhere above? Why did you
> change <self> indexing? AFAIU, after creating the *right* <arg> variable
> everything should work with no other changes.

Just following Lua 5.2 test suite behaviour. Your solution is good too.

> 
> >  assert(t:f(1,4) == 3 and t:f(2) == 11)
> >  print('+')
> >  
> > -- 
> > 2.31.0
> > 
> 
> -- 
> Best regards,
> IM

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-April/023220.html

-- 
Best regards,
Sergey Kaplun