Re: [Tarantool-patches] [PATCH luajit 1/2] test: support number value of tag in LuaJIT-tests

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

Hi, Sergey!
Thanks for the review!
See my answers below.

On 06.12.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch!
> LGTM with minor comment below.
> 
> On 16:42 Thu 05 Dec , Sergey Kaplun wrote:
> >The LuaJIT-tests suite lacks the ability to specify tags with custom
> >values. This patch adds the ability to specify number tags in the format
> >`+tag=number`. It is useful for version specification of system
> >libraries (like libc).
> 
> Usually, for such janitorial patches we add a note like
> "Required by the following patch".

Added the following line:
| Required for the next patch.

> 
> >---
> > test/LuaJIT-tests/test.lua | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/test/LuaJIT-tests/test.lua b/test/LuaJIT-tests/test.lua
> >index ab178331..2a535605 100644
> >--- a/test/LuaJIT-tests/test.lua
> >+++ b/test/LuaJIT-tests/test.lua
> >@@ -161,7 +161,12 @@ local function parse_args(t)
> >         error(arg .." does not expect an argument")
> >       end
> >     elseif arg:find"^[-+]" then
> >-      opts.tags[arg:sub(2)] = (arg:sub(1, 1) == "+")
> >+      local tagval = arg:find"="
> >+      if tagval then
> >+        opts.tags[arg:sub(2, tagval - 1)] = tonumber(arg:sub(tagval + 1))
> 
> I would also check a type here:
> 
> --- a/test/LuaJIT-tests/test.lua
> +++ b/test/LuaJIT-tests/test.lua
> @@ -162,7 +162,7 @@ local function parse_args(t)
>         end
>       elseif arg:find"^[-+]" then
>         local tagval = arg:find"="
> -      if tagval then
> +      if tagval and type(tagval) == 'number' then
>           opts.tags[arg:sub(2, tagval - 1)] = tonumber(arg:sub(tagval + 1))
>         else
>           opts.tags[arg:sub(2)] = (arg:sub(1, 1) == "+")

This check is excess, see the `string.find()` documentation [1]:
| If it finds a match, then `find` returns the indices of `s` where this
| occurrence starts and ends; otherwise, it returns `nil`.

We have only two options here: a number or `nil`, so this `if` check is
just enough.

> 
> >+      else
> >+        opts.tags[arg:sub(2)] = (arg:sub(1, 1) == "+")
> >+      end
> >     elseif arg:find"^%d+$" then
> >       if not opts.numbers_to_run then
> >         opts.numbers_to_run = {}
> >-- 
> >2.47.0
> >
> 
> -- 
> sergeyb@

[1]: https://www.lua.org/manual/5.1/manual.html#pdf-string.find

-- 
Best regards,
Sergey Kaplun