Re: [Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

On 15.03.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 15.03.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch!
> > 
> > This is why it's so important to take the suite intact at first: I
> > didn't even notice these changes in the previous series and now I regret
> > a bit regarding the solution with LUAJIT_TEST_INIT: for this case it
> > occurs to be ugly a little. Nevertheless, I believe Francois would be
> > interested in this use case, so it's worth to file a bug against
> > lua-Harness, IMHO.
> 
> I don't think so -- usage [1] declares the way to launch the suite.
> It is our problem, that we don't use it.

OK, I'll do it myself then.

> 

<snipped>

> > > diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
> > > index 08a99b8..5ce95e6 100644
> > > --- a/test/lua-Harness-tests/tap.lua
> > > +++ b/test/lua-Harness-tests/tap.lua
> > > @@ -195,6 +195,15 @@ function todo (reason, count)
> > >      todo_reason = reason
> > >  end
> > >  
> > > +-- The last arg element is guaranteed name of tested binary.
> > 
> > Typo: s/last/least/.
> > Typo: s/guaranteed name of tested/guaranteed to be the name of the tested/.
> 
> Thanks, update the branch. See the iterative patch below.
> ===================================================================
> diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
> index 5ce95e6..e527687 100644
> --- a/test/lua-Harness-tests/tap.lua
> +++ b/test/lua-Harness-tests/tap.lua
> @@ -195,7 +195,8 @@ function todo (reason, count)
>      todo_reason = reason
>  end
>  
> --- The last arg element is guaranteed name of tested binary.
> +-- The least arg element is guaranteed to be the name
> +-- of the tested binary.
>  function get_lua_binary_name ()
>      local i = 0
>      while arg[i] do
> ===================================================================
> 
> > 
> > > +function get_lua_binary_name ()
> > > +    local i = 0
> > > +    while arg[i] do
> > 
> > You make an excess lookup to a _G for each iteration. It's better to
> > pass <arg> as an argument to this function. Furthermore, it makes the
> > signature clearer, so user knows you're using <arg> to detect the
> > interpreter name.
> 
> This economy looks unnecessary -- there are only 6 calls to this
> function from the whole suite. You can provide time measurement
> to prove me wrong.
> I disagreed that it makes signature clearer, because there is no way to
> use something instead `arg` variable anyway. Please, give me an
> example, when usage of custom array is preferable than usage `arg`
> variable. If this function *always* uses `arg` variable only, why we
> should move it outside this function?

I forgot to add "feel free to ignore" here. You can write the code you
like and I can't make you to do it another way alone. For this purpose
there are two reviewers. I'll postpone my LGTM a bit for this change.

Regarding "no way to use something instead arg": nobody can stop you
from parsing shebang.

> 
> > 
> > > +        i = i - 1
> > > +    end
> > > +    return arg[i + 1]
> > > +end
> > > +
> > >  --
> > >  -- Copyright (c) 2009-2018 Francois Perrad
> > >  --
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://fperrad.frama.io/lua-Harness/usage/
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM