[Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT
sergos at tarantool.org
Tue Mar 16 17:25:16 MSK 2021
Thanks for the patch and review!
> On 16 Mar 2021, at 00:27, Igor Munkin <imun at tarantool.org> wrote:
> On 15.03.21, Sergey Kaplun wrote:
>> Thanks for the review!
>> On 15.03.21, Igor Munkin wrote:
>>> 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  declares the way to launch the suite.
>> It is our problem, that we don't use it.
> OK, I'll do it myself then.
>>>> 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
>>>> +-- 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
>> --- 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?
>>>> + i = i - 1
>>>> + end
>>>> + return arg[i + 1]
>>>> -- Copyright (c) 2009-2018 Francois Perrad
>>> Best regards,
>> : https://fperrad.frama.io/lua-Harness/usage/
>> Best regards,
>> Sergey Kaplun
> Best regards,
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Tarantool-patches