Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper GC root.

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

Hi, Sergey!
Thanks for the fixes!

On 23.07.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> fixes applied and force-pushed.
> 
> Sergey
> 
> On 10.07.2024 17:08, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the fixes!
> > LGTM, after fixing minor comments below.
> >
> > On 10.07.24, Sergey Bronnikov wrote:
> >> Hi, Sergey
> >>
> >> thanks for review. Fixes applied and force-pushed.
> >>
> >> Sergey
> >>
> >>
> >> On 09.07.2024 15:14, Sergey Kaplun via Tarantool-patches wrote:
> >>> Hi, Sergey!
> >>> Thanks for the patch!
> >>> Please consider my comments below.
> >>>
> >>> On 09.07.24, Sergey Bronnikov wrote:

<snipped>

> >>>> diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
> >>>> index c388c6a7..259528cb 100644
> >>>> --- a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
> >>>> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c

<snipped>

> >>>> +	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
> > I suggest renaming "chunk" to the "test_chunk" here too.
> 
> Fixed, but after this the line becomes longer max length and I need to 
> split it for two lines:
> 
> --- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
> +++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c
> @@ -83,7 +83,8 @@ unmarked_finalizer_tab_gcmark(void *test_state)
>          /* Not trigger GC during `lua_openffi()`. */
>          lua_gc(L, LUA_GCSTOP, 0);
> 
> -       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
> +       int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
> +                                  "test_chunk", "t");
>          if (res != LUA_OK) {
>                  test_comment("error loading Lua chunk: %s", 
> lua_tostring(L, -1));
>                  bail_out("error loading Lua chunk");
> 
> I would leave "chunk" due to this. And you?

OK, let's leave it as "chunk" in both places.

> 
> 
> >
> > Also, please add here comment about `sizeof(buff) - 1` too.

Please add a comment that the terminating '\0' is considered by parser
as part of the input, so we must chomp it.

> >

<snipped>

-- 
Best regards,
Sergey Kaplun