Re: [Tarantool-patches] [PATCH luajit 06/25] test: enable <ffi_callback.lua> in LuaJIT-tests

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

Hi, Sergey!
Thanks for the review!

Please consider my answers below.

On 23.01.24, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> thanks for the patch!
> 
> see comments below
> 
> On 1/19/24 14:32, Sergey Kaplun wrote:
> > This patch names all subtests and includes the test in <index>.
> > The test with calls with 10 arguments with sizes less than 8 bits are
> > dummy for M1. This suite lacks a mechanism to skip subtests satisfying
> > complicated conditions, so it is done manually by the corresponding `if`
> > check.
> >
> > Part of tarantool/tarantool#9398
> > ---
> >   test/LuaJIT-tests/lib/ffi/ffi_callback.lua | 33 +++++++++++-----------
> >   test/LuaJIT-tests/lib/ffi/index            |  1 +
> >   2 files changed, 18 insertions(+), 16 deletions(-)
> >
> > diff --git a/test/LuaJIT-tests/lib/ffi/ffi_callback.lua b/test/LuaJIT-tests/lib/ffi/ffi_callback.lua
> > index 1fd14bd0..253bf1d9 100644
> > --- a/test/LuaJIT-tests/lib/ffi/ffi_callback.lua
> > +++ b/test/LuaJIT-tests/lib/ffi/ffi_callback.lua
> > @@ -6,7 +6,7 @@ void qsort(void *base, size_t nmemb, size_t size,
> >   	   int (*compar)(const uint8_t *, const uint8_t *));
> >   ]]
> >   
> > -do
> > +do --- blacklisted callback
> 
> first letter should be uppercase and dot is missed in a comment, right?
> 
> here and below

As you mentioned, it is a test name, not a usual comment.
Also, there is no strict style for test names in this suite -- there are
with the uppercase first letter or lowcase, with a dot at the end of the
sentence, and without.

Welcome to the Wild West.

> 
> >     local cb = ffi.cast("int (*)(int, int, int)", function(a, b, c)
> >       return a+b+c
> >     end)
> > @@ -19,7 +19,7 @@ do
> >     end
> >   end
> >   
> > -do
> > +do --- cast to function
> >     assert(ffi.cast("int64_t (*)(int64_t, int64_t, int64_t)", function(a, b, c)
> >         return a+b+c
> >       end)(12345678901234567LL, 70000000000000001LL, 10000000909090904LL) ==
> > @@ -37,11 +37,13 @@ do
> >         return a+b+c
> >       end)(7.125, -123.25, 9999.33) == 9883.205078125)
> >   
> > -  assert(ffi.cast("int (*)(int, int, int, int, int, int, int, int, int, int)",
> > -    function(a, b, c, d, e, f, g, h, i, j)
> > -      return a+b+c+d+e+f+g+h+i+j
> > -    end)(-42, 17, 12345, 9987, -100, 11, 51, 0x12345678, 338, -78901234) ==
> > -    -42+17+12345+9987-100+11+51+0x12345678+338-78901234)
> > +  if not (jit.os == "OSX" and jit.arch == "arm64") then -- NYI
> > +    assert(ffi.cast("int (*)(int, int, int, int, int, int, int, int, int, int)",
> > +      function(a, b, c, d, e, f, g, h, i, j)
> > +        return a+b+c+d+e+f+g+h+i+j
> > +      end)(-42, 17, 12345, 9987, -100, 11, 51, 0x12345678, 338, -78901234) ==
> > +      -42+17+12345+9987-100+11+51+0x12345678+338-78901234)
> 
> My eyes are bleeding I can't read anymore...
> 
> Why you cannot add more whitespaces to expressions?

Obviously, there is room to improve this suite.
Also, we don't refactor the whole test suite. It is not the point of
this patchset, only enable some tests.

This suite is just a good reference that Mike's (or ours) patches don't
breake some parts of the LuaJIT. In the future we may reorganise these
tests inside our own suite, for example. Now we have no resources for
this, unfortunately.

> 
> 
> > +  end

<snipped>

-- 
Best regards,
Sergey Kaplun