Re: [Tarantool-patches] [PATCH luajit] test: adapt test checking global environment

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

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