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

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

[imeevma]

This patch replaces tonumber64() with misc.tonumber(), which works in a
similar way, but always returns 64-bit integer cdata.

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

@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')
 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
+
 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)
-{
-	luaL_checkany(L, 1);
-	int base = luaL_optint(L, 2, -1);
-	luaL_argcheck(L, (2 <= base && base <= 36) || base == -1, 2,
-		      "base out of range");
-	switch (lua_type(L, 1)) {
-	case LUA_TNUMBER:
-		base = (base == -1 ? 10 : base);
-		if (base != 10)
-			return luaL_argerror(L, 1, "string expected");
-		lua_settop(L, 1); /* return original value as is */
-		return 1;
-	case LUA_TSTRING:
-	{
-		size_t argl = 0;
-		const char *arg = luaL_checklstring(L, 1, &argl);
-		/* Trim whitespaces at begin/end */
-		while (argl > 0 && isspace(arg[argl - 1])) {
-			argl--;
-		}
-		while (isspace(*arg)) {
-			arg++; argl--;
-		}
-
-		/*
-		 * Check if we're parsing custom format:
-		 * 1) '0x' or '0X' trim in case of base == 16 or base == -1
-		 * 2) '0b' or '0B' trim in case of base == 2  or base == -1
-		 * 3) '-' for negative numbers
-		 * 4) LL, ULL, LLU - trim, but only for base == 2 or
-		 *    base == 16 or base == -1. For consistency do not bother
-		 *    with any non-common bases, since user may have specified
-		 *    base >= 22, in which case 'L' will be a digit.
-		 */
-		char negative = 0;
-		if (arg[0] == '-') {
-			arg++; argl--;
-			negative = 1;
-		}
-		if (argl > 2 && arg[0] == '0') {
-			if ((arg[1] == 'x' || arg[1] == 'X') &&
-			    (base == 16 || base == -1)) {
-				base = 16; arg += 2; argl -= 2;
-			} else if ((arg[1] == 'b' || arg[1] == 'B') &&
-			           (base == 2 || base == -1)) {
-				base = 2;  arg += 2; argl -= 2;
-			}
-		}
-		bool ull = false;
-		if (argl > 2 && (base == 2 || base == 16 || base == -1)) {
-			if (arg[argl - 1] == 'u' || arg[argl - 1] == 'U') {
-				ull = true;
-				--argl;
-			}
-			if ((arg[argl - 1] == 'l' || arg[argl - 1] == 'L') &&
-			    (arg[argl - 2] == 'l' || arg[argl - 2] == 'L'))
-				argl -= 2;
-			else {
-				ull = false;
-				goto skip;
-			}
-			if (!ull && (arg[argl - 1] == 'u' ||
-				     arg[argl - 1] == 'U')) {
-				ull = true;
-				--argl;
-			}
-		}
-skip:		base = (base == -1 ? 10 : base);
-		errno = 0;
-		char *arge;
-		unsigned long long result = strtoull(arg, &arge, base);
-		if (errno == 0 && arge == arg + argl) {
-			if (argl == 0) {
-				lua_pushnil(L);
-			} else if (negative) {
-				/*
-				 * To test overflow, consider
-				 *  result > -INT64_MIN;
-				 *  result - 1 > -INT64_MIN - 1;
-				 * Assumption:
-				 *  INT64_MAX == -(INT64_MIN + 1);
-				 * Finally,
-				 *  result - 1 > INT64_MAX;
-				 */
-				if (ull)
-					luaL_pushuint64(L, (UINT64_MAX - result) + 1);
-				else if (result != 0 && result - 1 > INT64_MAX)
-					lua_pushnil(L);
-				else
-					luaL_pushint64(L, -result);
-			} else {
-				luaL_pushuint64(L, result);
-			}
-			return 1;
-		}
-		break;
-	} /* LUA_TSTRING */
-	case LUA_TCDATA:
-	{
-		base = (base == -1 ? 10 : base);
-		if (base != 10)
-			return luaL_argerror(L, 1, "string expected");
-		uint32_t ctypeid = 0;
-		luaL_checkcdata(L, 1, &ctypeid);
-		if (ctypeid >= CTID_INT8 && ctypeid <= CTID_DOUBLE) {
-			lua_pushvalue(L, 1);
-			return 1;
-		}
-		break;
-	} /* LUA_TCDATA */
-	}
-	lua_pushnil(L);
-	return 1;
-}
-
 /* }}} */
 
 /**
@@ -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')
 ---
 - 123
 ...
-type(tonumber64('4294967296')) == 'number'
+type(tonumber64('4294967296')) == 'cdata'
 ---
 - true
 ...
@@ -329,7 +329,7 @@ tonumber64(tonumber64(2))
 ...
 tostring(tonumber64(tonumber64(3)))
 ---
-- '3'
+- 3ULL
 ...
 --  A test case for Bug#1131108 'tonumber64 from negative int inconsistency'
 tonumber64(-1)
@@ -364,11 +364,11 @@ tonumber64(-1.0)
 ---
 - -1
 ...
-tostring(tonumber64('1234567890123')) == '1234567890123'
+tostring(tonumber64('1234567890123')) == '1234567890123ULL'
 ---
 - true
 ...
-tostring(tonumber64('12345678901234')) == '12345678901234'
+tostring(tonumber64('12345678901234')) == '12345678901234ULL'
 ---
 - true
 ...
@@ -403,7 +403,7 @@ tonumber64('-9223372036854775809') == nil
 ---
 - true
 ...
-tostring(tonumber64('0')) == '0'
+tostring(tonumber64('0')) == '0ULL'
 ---
 - true
 ...
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
@@ -105,7 +105,7 @@ tonumber64()
 tonumber64('invalid number')
 tonumber64(123)
 tonumber64('123')
-type(tonumber64('4294967296')) == 'number'
+type(tonumber64('4294967296')) == 'cdata'
 tonumber64('9223372036854775807') == tonumber64('9223372036854775807')
 tonumber64('9223372036854775807') - tonumber64('9223372036854775800')
 tonumber64('18446744073709551615') == tonumber64('18446744073709551615')
@@ -125,8 +125,8 @@ tonumber64(-1ULL)
 -1ULL
 tonumber64(-1.0)
 6LL - 7LL
-tostring(tonumber64('1234567890123')) == '1234567890123'
-tostring(tonumber64('12345678901234')) == '12345678901234'
+tostring(tonumber64('1234567890123')) == '1234567890123ULL'
+tostring(tonumber64('12345678901234')) == '12345678901234ULL'
 tostring(tonumber64('123456789012345')) == '123456789012345ULL'
 tostring(tonumber64('1234567890123456')) == '1234567890123456ULL'
 
@@ -138,7 +138,7 @@ tostring(tonumber64('18446744073709551615')) == '18446744073709551615ULL'
 tonumber64('18446744073709551616') == nil
 tostring(tonumber64('-9223372036854775808')) == '-9223372036854775808LL'
 tonumber64('-9223372036854775809') == nil
-tostring(tonumber64('0')) == '0'
+tostring(tonumber64('0')) == '0ULL'
 
 --
 -- gh-3431: tonumber of strings with ULL.
-- 
2.25.1

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

[Sergey Kaplun]

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