Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Максим Корякшин" <m.kokryashkin@tarantool.org>
Cc: Maxim Kokryashkin <max.kokryashkin@gmail.com>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] test: adapt test checking global environment
Date: Thu, 27 Jan 2022 01:33:53 +0300	[thread overview]
Message-ID: <YfHMUfhkSYt7V7wv@tarantool.org> (raw)
In-Reply-To: <1642462945.143863816@f326.i.mail.ru>

Max,

Thanks for the patch!

LGTM, except a couple of typos.

On 18.01.22, Максим Корякшин wrote:
> 
> Hello, Sergey!
> Thanks for the review!
>  
> Here is the new commit message:
> ================================================================================
> test: adapt test checking global environment
>  
> LuaJIT doesn't take into account tail calls for
> call-level counting, so `getfenv()` behaviour is different
> in tail calls. getfenv()` default level is 1 which is invalid for

Typo: s/getfenv()`/`getfenv()`/.

> the test case when is called from tail call (`lj_debug_frame()`
> returns NULL).
>  
> This commits adds local variables, so there is no tail call any more.
>  
> Part of tarantool/tarantool#5870
> Resolves tarantool/tarantool#5713

Typo: "Resolves" should go prior to all other GitHub tags ("Part of" in
our case).

> ================================================================================
>  
> And the diff for the comment fix:
> ================================================================================
> diff --git a/test/PUC-Rio-Lua-5.1-tests/closure.lua b/test/PUC-Rio-Lua-5.1-tests/closure.lua
> index 86f6ad87..c4491dcf 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/closure.lua
> +++ b/test/PUC-Rio-Lua-5.1-tests/closure.lua
> @@ -170,9 +170,10 @@ local function foo (a)
>  
>  -- LuaJIT doesn't take into account tail calls for
>  -- call-level counting, so `getfenv()` behaviour is different
> --- in tail calls. For example, `pcall()` to this functon returns false
> --- in case of tailcall, because `getfenv()` default level is 1 which
> --- is invalid for this case (`lj_debug_frame()` returns NULL).
> +-- in tail calls. For example, `pcall()` to this functon returns

Typo: s/functon/function/.

> +-- false in case of tailcall, because `getfenv()` default level

Typo: "tailcall", but "tail calls". Please choose one option for
consistency (at least within a single comment).

> +-- is 1 which is invalid for this case (`lj_debug_frame()`
> +-- returns NULL).
>  -- See also https://github.com/tarantool/tarantool/issues/5713.
>    local env = getfenv()
>    return env
> ================================================================================
>  
> Best regards,
> Maxim Kokryashkin
>  
> > 
> >>Hi, Maxim!
> >>
> >>Thanks for the fixes!
> >>
> >>LGTM, except a few nits below.
> >>
> >>On 06.10.21, Maxim Kokryashkin wrote:
> >>> LuaJIT doesn't take into account tail calls for
> >>> call-level counting, so `getfenv()` behaviour is different
> >>> in tail calls. getfenv()` default level is 1 which is invalid for
> >>> the test case when is called from tail call (`lj_debug_frame()`
> >>> returns NULL).
> >>>
> >>> This commits adds local varibles, so there is no tail call any more.
> >>
> >>Typo: s/varibles/variables/
> >>
> >>>
> >>> Part of tarantool/tarantool#5870
> >>
> >>It's OK to mention that this patch "Resolves tarantool/tarantool#5713".
> >>
> >>> ---
> >>> Issue:  https://github.com/tarantool/tarantool/issues/5713
> >>> GitHub branch:  https://github.com/tarantool/luajit/tree/fckxorg/gh-5713-adapt-test-global-environment-PUC-Rio
> >>> CI:  https://github.com/tarantool/tarantool/tree/fckxorg/gh-5713-adapt-test-global-environment-PUC-Rio
> >>>
> >>> test/PUC-Rio-Lua-5.1-tests/closure.lua | 21 +++++++++++----------
> >>> 1 file changed, 11 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/test/PUC-Rio-Lua-5.1-tests/closure.lua b/test/PUC-Rio-Lua-5.1-tests/closure.lua
> >>> index 551fe70d..86f6ad87 100644
> >>> --- a/test/PUC-Rio-Lua-5.1-tests/closure.lua
> >>> +++ b/test/PUC-Rio-Lua-5.1-tests/closure.lua
> >>> @@ -167,22 +167,23 @@ local function foo (a)
> >>> assert(getfenv(0) == a)
> >>> assert(getfenv(1) == _G)
> >>> assert(getfenv(loadstring"") == a)
> >>> - return getfenv()
> >>> +
> >>> +-- LuaJIT doesn't take into account tail calls for
> >>> +-- call-level counting, so `getfenv()` behaviour is different
> >>> +-- in tail calls. For example, `pcall()` to this functon returns false
> >>> +-- in case of tailcall, because `getfenv()` default level is 1 which
> >>> +-- is invalid for this case (`lj_debug_frame()` returns NULL).
> >>> +-- See also  https://github.com/tarantool/tarantool/issues/5713 .
> >>
> >>Minor: linewidth for comments is 66 symbols.
> >>
> >>> + local env = getfenv()
> >>> + return env
> >>> end
> >>>
> >>> f = coroutine.wrap(foo)
> >>> local a = {}
> >>> assert(f(a) == _G)
> >>> local a,b = pcall(f)
> >>> --- FIXME: LuaJIT doesn't take into account tail calls for
> >>> --- call-level counting, so `getfenv()` behaviour is different
> >>> --- in tail calls. For example, this `pcall()` returns false,
> >>> --- because `getfenv()` default level is 1 which is invalid for
> >>> --- this case when is called from tail call (`lj_debug_frame()`
> >>> --- returns NULL).
> >>> --- See also  https://github.com/tarantool/tarantool/issues/5713 .
> >>> --- Test is disabled for LuaJIT for now.
> >>> --- assert(a and b == _G)
> >>> +-- Test is adapted to the behavior LuaJIT. See the comment above.

Typo: either s/behavior LuaJIT/LuaJIT behavior/ or
s/behavior LuaJIT/behavior of LuaJIT/ for this case.

> >>> +assert(a and b == _G)
> >>>
> >>>
> >>> -- tests for multiple yield/resume arguments
> >>> --
> >>> 2.33.0
> >>>
> >>
> >>--
> >>Best regards,
> >>Sergey Kaplun
> > 

-- 
Best regards,
IM

  reply	other threads:[~2022-01-26 22:35 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 13:04 Maxim Kokryashkin via Tarantool-patches
2021-12-16 14:34 ` Sergey Kaplun via Tarantool-patches
2022-01-17 23:42   ` Максим Корякшин via Tarantool-patches
2022-01-26 22:33     ` Igor Munkin via Tarantool-patches [this message]
2022-02-07  9:32       ` Максим Корякшин via Tarantool-patches
2022-01-17 23:43   ` Максим Корякшин via Tarantool-patches
2022-02-17 16:34 ` Igor Munkin via Tarantool-patches
  -- strict thread matches above, loose matches on Subject: below --
2021-09-24 18:20 Maxim Kokryashkin via Tarantool-patches
2021-10-05 17:37 ` Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YfHMUfhkSYt7V7wv@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --subject='Re: [Tarantool-patches] [PATCH luajit] test: adapt test checking global environment' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox