From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 8A45D6EC56; Tue, 16 Mar 2021 17:25:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8A45D6EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615904718; bh=1ti1TKh+YKJ6RNIfzJm52FAn6BYZYxXxrW2MttgjuMw=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=AOPOyn/iKbVZ0LtJUvsXBT/KhYUurm3anpJYWrglX0tbMcEZ28R37GpKNIptKUVL1 XNszorV8StEH1gz5HkG2Fi54bUD6GA2Mws/GqeH0hU9eKTy82psYwIjJNjx5IRNPXC I4yAhDy4BQPI5j2ol/b1evYDPBcfvKhOyNzTFKHM= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 165676EC56 for ; Tue, 16 Mar 2021 17:25:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 165676EC56 Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1lMAdI-00035P-AD; Tue, 16 Mar 2021 17:25:16 +0300 Message-Id: <864C77C4-C342-41FC-A7A0-0ACAF072F0C4@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_E8473AAF-3B83-4FBA-84A8-B12EEA1867C5" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Date: Tue, 16 Mar 2021 17:25:16 +0300 In-Reply-To: <20210315212710.GJ9042@tarantool.org> To: Sergey Kaplun References: <4e3445b32f5bcf94acc7c87c360aeb68ff0c399c.1615819534.git.skaplun@tarantool.org> <20210315173950.GF9042@tarantool.org> <20210315183330.GC16737@root> <20210315212710.GJ9042@tarantool.org> X-Mailer: Apple Mail (2.3654.60.0.2.21) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96485A7A9FC131893E945045C9B0C221EF6B11A7C645D09F6182A05F5380850402061ABD6EE04AC49D56A6C17531BFD373DB0A258ED8D68EC916A18F67CE31C64 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7227E4400968B082FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637EDC9855826FBDF5A8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CF89CA98302ED496F02CE7803932730C1D785A724A3E85DD1A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7C5C8BF3F796D5A11793EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6ACE00135B021D8CA089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E702E4EE3A04994FF497E5BFE6E7EFDEDCD789D4C264860C145E X-C1DE0DAB: 0D63561A33F958A591A652BFD41FCCEC3964AB5B1E38FEBDF00181F0C3878FB0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F38194B2C99DC1288097C724A438DD3F79BDAF55FB473B92728B6C77CD6AD51F8DD693A29EE6155C1D7E09C32AA3244C9C13DD5A145F2AD21464B5D881B76E8E725D5B54B2FE4575FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojwV/GgY7Z4vUPQ4vYErqjHA== X-Mailru-Sender: 3B9A0136629DC912F4AABCEFC589C81EE4DCD127B0C2540EB059941E60392A7F548B937F2C4D21ADAD07DD1419AC565FA614486B47F28B67C5E079CCF3B0523AED31B7EB2E253A9E112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Ostanevich via Tarantool-patches Reply-To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_E8473AAF-3B83-4FBA-84A8-B12EEA1867C5 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thanks for the patch and review! > On 16 Mar 2021, at 00:27, Igor Munkin wrote: >=20 > Sergey, >=20 > On 15.03.21, Sergey Kaplun wrote: >> Igor, >>=20 >> Thanks for the review! >>=20 >> On 15.03.21, Igor Munkin wrote: >>> Sergey, >>>=20 >>> Thanks for the patch! >>>=20 >>> 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. >>=20 >> I don't think so -- usage [1] declares the way to launch the suite. >> It is our problem, that we don't use it. >=20 > OK, I'll do it myself then. >=20 >>=20 >=20 > >=20 >>>> 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 =3D reason >>>> end >>>>=20 >>>> +-- The last arg element is guaranteed name of tested binary. >>>=20 >>> Typo: s/last/least/. >>> Typo: s/guaranteed name of tested/guaranteed to be the name of the = tested/. >>=20 >> Thanks, update the branch. See the iterative patch below. >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> 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 =3D reason >> end >>=20 >> --- 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 =3D 0 >> while arg[i] do >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>=20 >>>=20 >>>> +function get_lua_binary_name () >>>> + local i =3D 0 >>>> + while arg[i] do >>>=20 >>> You make an excess lookup to a _G for each iteration. It's better to >>> pass as an argument to this function. Furthermore, it makes = the >>> signature clearer, so user knows you're using to detect the >>> interpreter name. >>=20 >> 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? >=20 > 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. >=20 > Regarding "no way to use something instead arg": nobody can stop you > from parsing shebang. >=20 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=20 outside the function, hence if the code is not in a trace you=E2=80=99ll = 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 >>=20 >>>=20 >>>> + i =3D i - 1 >>>> + end >>>> + return arg[i + 1] >>>> +end >>>> + >>>> -- >>>> -- Copyright (c) 2009-2018 Francois Perrad >>>> -- >>>> --=20 >>>> 2.28.0 >>>>=20 >>>=20 >>> --=20 >>> Best regards, >>> IM >>=20 >> [1]: https://fperrad.frama.io/lua-Harness/usage/ >>=20 >> --=20 >> Best regards, >> Sergey Kaplun >=20 > --=20 > Best regards, > IM --Apple-Mail=_E8473AAF-3B83-4FBA-84A8-B12EEA1867C5 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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 =3D 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.
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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 =3D 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 =3D 0
    while arg[i] do
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


+function = get_lua_binary_name ()
+    local i =3D = 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=E2=80=99ll 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 = =3D 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

= --Apple-Mail=_E8473AAF-3B83-4FBA-84A8-B12EEA1867C5--