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, again!

On 23.01.24, Sergey Kaplun via Tarantool-patches wrote:
> 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.

We discussed offline adding the additional padding to make the result
more readable. Like the following:

| call(-42.5,  17.125,  ... ) ==
|      -42.5 + 17.125 + ...

I'll fix this in the next patchset version.

> 
> 
> > 
> > 
> > > +    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

-- 
Best regards,
Sergey Kaplun