Re: [Tarantool-patches] [PATCH luajit 05/25] test: enable <ffi_call.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!
> 
> please take a look on my comments
> 
> 
> Sergey
> 
> On 1/19/24 14:32, Sergey Kaplun wrote:
> > This patch removes unused `dofile()`. Also, it uses the basename of the
> > ctest library to be loaded via ffi. It adds groups with names of
> > subtests and enables the test in <index>. Complex type is undefined on
> > Windows, so the correponding tests are not enabled for this platform.
> > Fastcalls are enabled only for the x86 architecture. Stdcalls are
> > enabled only for Windows on x86. Tests 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_call.lua | 180 +++++++++++++------------
> >   test/LuaJIT-tests/lib/ffi/index        |   1 +
> >   2 files changed, 92 insertions(+), 89 deletions(-)
> >
> > diff --git a/test/LuaJIT-tests/lib/ffi/ffi_call.lua b/test/LuaJIT-tests/lib/ffi/ffi_call.lua
> > index 1eb5e906..c362f3e0 100644
> > --- a/test/LuaJIT-tests/lib/ffi/ffi_call.lua
> > +++ b/test/LuaJIT-tests/lib/ffi/ffi_call.lua
> > @@ -1,8 +1,5 @@
> > -

<snipped>

> > +
> > +do --- call 10 args
> > +  if not (jit.os == "OSX" and jit.arch == "arm64") then -- NYI
> > +    assert(C.call_10i(-42, 17, 12345, 9987, -100, 11, 51, 0x12345678, 338, -78901234) == -42+17+12345+9987-100+11+51+0x12345678+338-78901234)
> 
> these tests totally unreadable.
> 
> I would rewrite such tests in the following manner:
> 
> local args = {-42, 17, 12345, 9987, -100, 11, 51, 0x12345678, 338, 
> -78901234}
> 
> assert(C.call_10i(unpack(args)) == sum(args))
> 
> where `sum` is a function that sum elements in a passed table.
> 
> (Or even calculate RHS once and use it and doesn't depend on 
> calculations in runtime.)
> 
> Same comment for other similar tests.

Obviously, there is room to improve this suite.
But as an agreement before: we don't change the semantics of this tests
(like you suggested by introducing the other function `sum()` that
may be compiled, for example).

Also, we don't refactor the whole test suite. It is not the point of
this patchset. 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.


> 
> 
> > +    assert(C.call_10f(-42.5, 17.125, 12345.5, 9987, -100.625, 11, 51, 0x123456, 338, -789012.75) == -42.5+17.125+12345.5+9987-100.625+11+51+0x123456+338-789012.75)
> > +  end
> > +  assert(C.call_10d(-42.5, 17.125, 12345.5, 9987, -100.625, 11, 51, 0x12345678, 338, -78901234.75) == -42.5+17.125+12345.5+9987-100.625+11+51+0x12345678+338-78901234.75)
> > +end
> >   
> > -do
> > +do --- call pointer arg

<snipped>

-- 
Best regards,
Sergey Kaplun