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

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

[-- Attachment #1: Type: text/plain, Size: 4027 bytes --]

Hi!

Thanks for the patch and review!


> On 16 Mar 2021, at 00:27, Igor Munkin <imun@tarantool.org> wrote:
> 
> 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 believe the comment to the get_lua_binary_name tells exactly how the
binary is detected. Other than that - you will need to access the arg 
outside the function, hence if the code is not in a trace you’ll have
to pass it on the stack, and if it is in a trace the optimizer will
CSE the arg access from within the function?

LGTM.

Sergos


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


[-- Attachment #2: Type: text/html, Size: 19227 bytes --]