Re: [Tarantool-patches] [PATCH v1 1/1] lua: replace tonumber64() with misc.tonumber64()

[Sergey Kaplun <skaplun@tarantool.org>]

Mergen,

Thanks for the patch! Please consider my questions and comments below.

On 16.11.20, imeevma@tarantool.org wrote:
> This patch replaces tonumber64() with misc.tonumber(), which works in a
> similar way, but always returns 64-bit integer cdata.

This part should be describe more verbosely. Also see my comments at
[1].

> 
> Closes #4738
> ---
> https://github.com/tarantool/tarantool/issues/4738
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4738-move-tonumber64-to-luajit

Please fix red CI [2] before the next review.
I think that it's a good practice to add a link to CI for your branch
here too. It safes reviewers' time and you can check yourself again.

> 
> @ChangeLog
>  - Now tonumber64() always returns 64-bit integer cdata (gh-4738).
> 
>  src/box/lua/schema.lua |   3 +
>  src/lua/init.c         | 121 -----------------------------------------
>  test/box/misc.result   |  10 ++--
>  test/box/misc.test.lua |   8 +--
>  4 files changed, 12 insertions(+), 130 deletions(-)
> 
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index b77c07bc8..eb72b12a5 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1,6 +1,7 @@
>  -- schema.lua (internal file)
>  --
>  local ffi = require('ffi')
> +local misc = require('misc')

This "require" isn't required for this builtin library.

>  local msgpack = require('msgpack')
>  local fun = require('fun')
>  local log = require('log')
> @@ -11,6 +12,8 @@ local function setmap(table)
>      return setmetatable(table, { __serialize = 'map' })
>  end
>  
> +tonumber64 = misc.tonumber64

Why do you decide to redeclare this function here and not in another
file or inside `tarantool_lua_init()`?

> +
>  local builtin = ffi.C
>  
>  -- performance fixup for hot functions
> diff --git a/src/lua/init.c b/src/lua/init.c
> index a0b2fc775..a46dc6136 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -174,126 +174,6 @@ static const char *lua_modules[] = {
>   * {{{ box Lua library: common functions
>   */
>  
> -/**
> - * Convert lua number or string to lua cdata 64bit number.
> - */
> -static int
> -lbox_tonumber64(struct lua_State *L)
> -{

<snipped>

> -}
> -
>  /* }}} */
>  
>  /**
> @@ -446,7 +326,6 @@ tarantool_lua_init(const char *tarantool_bin, int argc, char **argv)
>  	/* Initialize ffi to enable luaL_pushcdata/luaL_checkcdata functions */
>  	luaL_loadstring(L, "return require('ffi')");
>  	lua_call(L, 0, 0);
> -	lua_register(L, "tonumber64", lbox_tonumber64);
>  
>  	tarantool_lua_utf8_init(L);
>  	tarantool_lua_utils_init(L);
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 714d5ee79..f6b72ccee 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -290,7 +290,7 @@ tonumber64('123')

Should all these tests be moved to LuaJIT test-suite?

>  ---
>  - 123
>  ...

<snipped>

> diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
> index 63cbe2bc5..c642cf95f 100644
> --- a/test/box/misc.test.lua
> +++ b/test/box/misc.test.lua

<snipped>

> -- 
> 2.25.1
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020955.html
[2]: https://gitlab.com/tarantool/tarantool/-/pipelines/216530551/failures

-- 
Best regards,
Sergey Kaplun