Re: [Tarantool-patches] [PATCH luajit 02/25] test: separate LuaJIT helpers from ffi_util.inc

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

Hi, Sergey!
Thanks for the review!
Added the comments to the fucntions and force-pushed the branch.

See also my answers below.

On 23.01.24, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> thanks for the patch!
> 
> On 1/19/24 14:32, Sergey Kaplun wrote:
> > This patch moves common helpers from <LuaJIT-tests/common/ffi_util.inc>
> > into separate files by analogy with <common/expect_error.lua>. The
> Why do you need to move to separate files? What is an idea behind it?

I suppose that this is "common" helpers with a bit different
functionality.

As mentioned I tried to respect the existed code style of the suite
here (i.e., it is done by analogy with <common/expect_error.lua>).

All of these files return just a single function to use inside a test.

> > `include()` helper isn't touched since it is used in system dependend
> > tests, which won't be modified for now.
> >
> > Part of tarantool/tarantool#9398
> > ---
> >   test/LuaJIT-tests/common/fails.lua          |  3 +++
> >   test/LuaJIT-tests/common/ffi/checkfail.lua  | 10 +++++++++
> >   test/LuaJIT-tests/common/ffi/checktypes.lua | 11 ++++++++++
> >   test/LuaJIT-tests/common/ffi_util.inc       | 23 ---------------------
> >   4 files changed, 24 insertions(+), 23 deletions(-)
> >   create mode 100644 test/LuaJIT-tests/common/fails.lua
> >   create mode 100644 test/LuaJIT-tests/common/ffi/checkfail.lua
> >   create mode 100644 test/LuaJIT-tests/common/ffi/checktypes.lua
> >
> > diff --git a/test/LuaJIT-tests/common/fails.lua b/test/LuaJIT-tests/common/fails.lua
> > new file mode 100644
> > index 00000000..d555a2b5
> > --- /dev/null
> > +++ b/test/LuaJIT-tests/common/fails.lua
> > @@ -0,0 +1,3 @@
> > +return function(f, ...)
> > +  if pcall(f, ...) ~= false then error("failure expected", 2) end
> > +end
> > diff --git a/test/LuaJIT-tests/common/ffi/checkfail.lua b/test/LuaJIT-tests/common/ffi/checkfail.lua
> > new file mode 100644
> > index 00000000..60d8449a
> > --- /dev/null
> > +++ b/test/LuaJIT-tests/common/ffi/checkfail.lua
> > @@ -0,0 +1,10 @@
> > +local ffi = require("ffi")
> > +
> please add a comment with function's description

Added the following comment:

===================================================================
diff --git a/test/LuaJIT-tests/common/ffi/checkfail.lua b/test/LuaJIT-tests/common/ffi/checkfail.lua
index 60d8449a..d3ca74e8 100644
--- a/test/LuaJIT-tests/common/ffi/checkfail.lua
+++ b/test/LuaJIT-tests/common/ffi/checkfail.lua
@@ -1,5 +1,9 @@
 local ffi = require("ffi")
 
+-- Checker that takes an array of strings that should represent
+-- different invalid CTypes (a more common pattern). Also, the
+-- second argument may be also the `loadstring` function to check
+-- invalid literals or `ffi.cdef` to check invalid C definitions.
 return function(t, f)
   f = f or ffi.typeof
   for i=1,1e9 do
===================================================================

> > +return function(t, f)
> > +  f = f or ffi.typeof
> > +  for i=1,1e9 do
> > +    local tp = t[i]
> > +    if not tp then break end
> > +    assert(pcall(f, tp) == false, tp)
> > +  end
> > +end
> > diff --git a/test/LuaJIT-tests/common/ffi/checktypes.lua b/test/LuaJIT-tests/common/ffi/checktypes.lua
> > new file mode 100644
> > index 00000000..6d748e3b
> > --- /dev/null
> > +++ b/test/LuaJIT-tests/common/ffi/checktypes.lua
> > @@ -0,0 +1,11 @@
> > +local ffi = require("ffi")
> > +
> please add a comment with function's description

===================================================================
diff --git a/test/LuaJIT-tests/common/ffi/checktypes.lua b/test/LuaJIT-tests/common/ffi/checktypes.lua
index 6d748e3b..c995d667 100644
--- a/test/LuaJIT-tests/common/ffi/checktypes.lua
+++ b/test/LuaJIT-tests/common/ffi/checktypes.lua
@@ -1,5 +1,9 @@
 local ffi = require("ffi")
 
+-- Checker that takes an array with the following each 3 elements:
+-- 1) Sizeof for the given C type to be checked.
+-- 2) Alignof for the given C type to be checked.
+-- 3) String representing the C type.
 return function(t)
   for i=1,1e9,3 do
     local tp = t[i+2]
===================================================================

> > +return function(t)
> > +  for i=1,1e9,3 do
> > +    local tp = t[i+2]
> > +    if not tp then break end
> > +    local id = ffi.typeof(tp)
> > +    assert(ffi.sizeof(id) == t[i], tp)
> > +    assert(ffi.alignof(id) == t[i+1], tp)
> 
> Why test function contains asserts()? Wouldn't be better to return 
> false/true and raise assert in test itself?

I have no perporse to refactor the whole suite. Also, I suppose there
are much more to do ;). `expect_error()` raises the error to, so see no
differences here.

> 
> same questions below
> 
> > +  end
> > +end
> > diff --git a/test/LuaJIT-tests/common/ffi_util.inc b/test/LuaJIT-tests/common/ffi_util.inc
> > index 1eee8dd9..9604d7b0 100644
> > --- a/test/LuaJIT-tests/common/ffi_util.inc
> > +++ b/test/LuaJIT-tests/common/ffi_util.inc
> > @@ -4,29 +4,6 @@
> >   
> >   local ffi = require("ffi")
> >   
> > -function checkfail(t, f)
> > -  f = f or ffi.typeof
> > -  for i=1,1e9 do
> > -    local tp = t[i]
> > -    if not tp then break end
> > -    assert(pcall(f, tp) == false, tp)
> > -  end
> > -end
> > -
> > -function checktypes(t)
> > -  for i=1,1e9,3 do
> > -    local tp = t[i+2]
> > -    if not tp then break end
> > -    local id = ffi.typeof(tp)
> > -    assert(ffi.sizeof(id) == t[i], tp)
> > -    assert(ffi.alignof(id) == t[i+1], tp)
> > -  end
> > -end
> > -
> > -function fails(f, ...)
> > -  if pcall(f, ...) ~= false then error("failure expected", 2) end
> > -end
> > -
> >   local incroot = os.getenv("INCROOT") or "/usr/include"
> >   local cdefs = os.getenv("CDEFS") or ""
> What for these env variables need?

They are unused for now, since the function `include()` below is not
used anywhere.
The first one should be used as a sysroot for finding C includes, and the
second one as an additional C defines.

> >   

-- 
Best regards,
Sergey Kaplun