From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B1CC8469710 for ; Wed, 25 Nov 2020 22:17:22 +0300 (MSK) Date: Wed, 25 Nov 2020 22:16:44 +0300 From: Sergey Kaplun Message-ID: <20201125191644.GB13309@root> References: <019d13b57364ded2f43e93efbc99a0b1ef11fc51.1605518956.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <019d13b57364ded2f43e93efbc99a0b1ef11fc51.1605518956.git.imeevma@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] lua: replace tonumber64() with misc.tonumber64() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.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) > -{ > -} > - > /* }}} */ > > /** > @@ -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 > ... > 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 > -- > 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