Hi! Thank you for review! I addressed your comments, and pushed the new version on the branch. The diff is below. It is rather big, but I feel like it belongs here, rather than in a `v2` letter, since the changes are very minor. -- Serge Petrenko sergepetrenko@tarantool.org > 21 июня 2019 г., в 18:53, Vladimir Davydov написал(а): > > On Wed, Jun 19, 2019 at 06:58:05PM +0300, Serge Petrenko wrote: >> Add a decimal library to lua. >> >> Part of #692 >> >> @TarantoolBot document >> Title: Document decimal module in lua. >> >> First of all, you have to require the package via >> `decimal = require('decimal')` >> Now you can construct decimals via `tonumber` method. >> Decimals may be constructed from lua numbers, strings, unsigned and >> signed 64 bit integers. >> Decimal is a fixed-point type with maximum 38 digits of precision. All >> the calculations are exact, so, be careful when constructing decimals >> from lua numbers: they may hold only 15 decimal digits of precision. >> You are advised to construct decimals from strings, since strings >> represent decimals exactly, and vice versa. >> >> ``` >> a = decimal.tonumber(123e-7) >> b = decimal.tonumber('123.456') >> c = decimal.tonumber('123.456e2') >> d = decimal.tonumber(123ULL) >> e = decimal.tonumber(2) >> ``` > > tonumber is a confusing name IMO. Let's rename it to decimal.new() Ok > >> The allowed operations are addition, subtraction, division, >> multiplication and power. If at least one of the operands is decimal, >> decimal operations are performed. The other operand may be either >> decimal or string, containing a number representation, or a lua number. >> When the operation is called as `decimal.opname`, both operands may be >> strings or lua numbers, e.g. `decimal.add(23, '123.456') == 146.456`: >> ``` > > Can these functions fail (overflow, underflow)? What happens on error? > Please mention in this document. I made the message more informative. > >> tarantool> a + b >> --- >> - '123.456012300000000' >> ... >> >> tarantool> decimal.add(a,b) >> --- >> - '123.456012300000000' >> ... >> >> tarantool> c - d >> --- >> - '12222.6' >> ... >> >> tarantool> decimal.sub(c,d) > > I don't think we need decimal.add/sub/div/mul methods as long as we have > corresponding operators. No problem. > >> The following math functions are also supported: >> log10, ln, exp, sqrt, pow. When specified as >> `decimal.opname()`, operations may be performed on >> strings and lua numbers. >> ``` >> f = decimal.tonumber(100) >> tarantool> f:log10() > > This looks weird. I think we should only allow decimal.log10(x), > not x:log10(). All the math functions are now accessed as decimal.func(), decimal.scale() and decimal.precision() are added. > >> --- >> src/CMakeLists.txt | 1 + >> src/lua/decimal.c | 327 ++++++++++++++++++++++++++++++++++++++ >> src/lua/decimal.h | 39 +++++ >> src/lua/init.c | 2 + >> test/app/decimal.result | 172 ++++++++++++++++++++ >> test/app/decimal.test.lua | 50 ++++++ >> 6 files changed, 591 insertions(+) >> create mode 100644 src/lua/decimal.c >> create mode 100644 src/lua/decimal.h >> create mode 100644 test/app/decimal.result >> create mode 100644 test/app/decimal.test.lua > > Please consider implementing this using Lua ffi. Take a look at > src/lua/uuid.lua for example. They say that ffi implementation is > generally faster than Lua C, because it doesn't break JIT traces. > It might be worth benchmarking the two approaches. I have written a proof-of-concept ffi implementation and tested its performance. It has shown a ~25% performance decrease over the current representation. The details are in the issue comments (https://github.com/tarantool/tarantool/issues/692#issuecomment-505043929) I propose to leave existing implementation as is. diff --git a/src/lua/decimal.c b/src/lua/decimal.c index 9e2fd41b5..4dc0dff7d 100644 --- a/src/lua/decimal.c +++ b/src/lua/decimal.c @@ -227,10 +227,10 @@ LDECIMAL_CMPOP(lt, <); LDECIMAL_CMPOP(le, <=); static int -ldecimal_tonumber(struct lua_State *L) +ldecimal_new(struct lua_State *L) { if (lua_gettop(L) < 1) - luaL_error(L, "Usage: decimal.tonumber(value)"); + luaL_error(L, "Usage: decimal.new(value)"); lua_todecimal(L, 1); decimal_t *lhs = lua_checkdecimal(L, 1); decimal_t *res = lua_pushdecimal(L); @@ -251,6 +251,28 @@ ldecimal_round(struct lua_State *L) return 1; } +static int +ldecimal_scale(struct lua_State *L) +{ + if (lua_gettop(L) < 1) + return luaL_error(L, "Usage: decimal.scale(decimal)"); + decimal_t *lhs = lua_checkdecimal(L, 1); + int scale = decimal_scale(lhs); + lua_pushnumber(L, scale); + return 1; +} + +static int +ldecimal_precision(struct lua_State *L) +{ + if (lua_gettop(L) < 1) + return luaL_error(L, "Usage: decimal.precisiion(decimal)"); + decimal_t *lhs = lua_checkdecimal(L, 1); + int precision = decimal_precision(lhs); + lua_pushnumber(L, precision); + return 1; +} + static int ldecimal_tostring(struct lua_State *L) { @@ -262,14 +284,6 @@ ldecimal_tostring(struct lua_State *L) } static const luaL_Reg ldecimal_mt[] = { - {"log10", ldecimal_log10}, - {"ln", ldecimal_ln}, - {"exp", ldecimal_exp}, - {"sqrt", ldecimal_sqrt}, - {"round", ldecimal_round}, - {"minus", ldecimal_minus}, - {"abs", ldecimal_abs}, - {"tostring", ldecimal_tostring}, {"__unm", ldecimal_minus}, {"__add", ldecimal_add}, {"__sub", ldecimal_sub}, @@ -284,23 +298,16 @@ static const luaL_Reg ldecimal_mt[] = { }; static const luaL_Reg ldecimal_lib[] = { - {"eq", ldecimal_eq}, - {"lt", ldecimal_lt}, - {"le", ldecimal_le}, - {"add", ldecimal_add}, - {"sub", ldecimal_sub}, - {"mul", ldecimal_mul}, - {"div", ldecimal_div}, {"log10", ldecimal_log10}, {"ln", ldecimal_ln}, {"pow", ldecimal_pow}, {"exp", ldecimal_exp}, {"sqrt", ldecimal_sqrt}, {"round", ldecimal_round}, - {"minus", ldecimal_minus}, + {"scale", ldecimal_scale}, + {"precision", ldecimal_precision}, {"abs", ldecimal_abs}, - {"tostring", ldecimal_tostring}, - {"tonumber", ldecimal_tonumber}, + {"new", ldecimal_new}, {NULL, NULL} }; diff --git a/test/app/decimal.result b/test/app/decimal.result index 93c1c7557..22590a082 100644 --- a/test/app/decimal.result +++ b/test/app/decimal.result @@ -5,69 +5,69 @@ test_run = require('test_run').new() --- ... -- check various constructors -decimal.tonumber('1234.5678') +decimal.new('1234.5678') --- - '1234.5678' ... -decimal.tonumber('1e6') +decimal.new('1e6') --- - '1000000' ... -decimal.tonumber('-6.234612e2') +decimal.new('-6.234612e2') --- - '-623.4612' ... -decimal.tonumber(tonumber64(2^63)) +decimal.new(tonumber64(2^63)) --- - '9223372036854775808.000000000000000' ... -decimal.tonumber(12345678ULL) +decimal.new(12345678ULL) --- - '12345678' ... -decimal.tonumber(-12345678LL) +decimal.new(-12345678LL) --- - '-12345678' ... -decimal.tonumber(1) +decimal.new(1) --- - '1.000000000000000' ... -decimal.tonumber(-1) +decimal.new(-1) --- - '-1.000000000000000' ... -decimal.tonumber(2^64) +decimal.new(2^64) --- - '18446744073709551616.000000000000000' ... -decimal.tonumber(2^(-20)) +decimal.new(2^(-20)) --- - '0.000000953674316' ... -a = decimal.tonumber('100') +a = decimal.new('100') --- ... -a:log10() +decimal.log10(a) --- - '2' ... -a:ln() +decimal.ln(a) --- - '4.6051701859880913680359829093687284152' ... -- overflow, e^100 > 10^38 -a:exp() +decimal.exp(a) --- - error: Operation failed ... -- e^87 < 10^38, no overflow, but rounds -- to 0 digits after the decimal point. -decimal.tonumber(87):exp() +decimal.exp(decimal.new(87)) --- - '60760302250568721495223289381302760753' ... -a:sqrt() +decimal.sqrt(a) --- - '10' ... @@ -75,26 +75,18 @@ a --- - '100' ... -a = decimal.tonumber('-123.456') +a = decimal.new('-123.456') --- ... -a:round(2) +decimal.round(a, 2) --- - '-123.46' ... -a:round(1) ---- -- '-123.5' -... -a:round(0) ---- -- '-123' -... -a:abs() +decimal.abs(a) --- - '123.456' ... -a:tostring() +a --- - '-123.456' ... @@ -122,11 +114,11 @@ a ^ 2 --- - '15241.383936' ... -a:abs() ^ 0.5 +decimal.abs(a) ^ 0.5 --- - '11.111075555498666484621494041182192341' ... -a:abs() ^ 0.5 == a:abs():sqrt() +decimal.abs(a) ^ 0.5 == decimal.sqrt(decimal.abs(a)) --- - true ... @@ -150,23 +142,3 @@ a == a --- - true ... -decimal.tostring(a) ---- -- '-123.456' -... -a:tostring() ---- -- '-123.456' -... -decimal.abs(a) ---- -- '123.456' -... -decimal.minus(a) ---- -- '123.456' -... -decimal.round(a, 2) ---- -- '-123.46' -... diff --git a/test/app/decimal.test.lua b/test/app/decimal.test.lua index 4751c1500..b101f07b6 100644 --- a/test/app/decimal.test.lua +++ b/test/app/decimal.test.lua @@ -2,49 +2,41 @@ decimal = require('decimal') test_run = require('test_run').new() -- check various constructors -decimal.tonumber('1234.5678') -decimal.tonumber('1e6') -decimal.tonumber('-6.234612e2') -decimal.tonumber(tonumber64(2^63)) -decimal.tonumber(12345678ULL) -decimal.tonumber(-12345678LL) -decimal.tonumber(1) -decimal.tonumber(-1) -decimal.tonumber(2^64) -decimal.tonumber(2^(-20)) +decimal.new('1234.5678') +decimal.new('1e6') +decimal.new('-6.234612e2') +decimal.new(tonumber64(2^63)) +decimal.new(12345678ULL) +decimal.new(-12345678LL) +decimal.new(1) +decimal.new(-1) +decimal.new(2^64) +decimal.new(2^(-20)) -a = decimal.tonumber('100') -a:log10() -a:ln() +a = decimal.new('100') +decimal.log10(a) +decimal.ln(a) -- overflow, e^100 > 10^38 -a:exp() +decimal.exp(a) -- e^87 < 10^38, no overflow, but rounds -- to 0 digits after the decimal point. -decimal.tonumber(87):exp() -a:sqrt() +decimal.exp(decimal.new(87)) +decimal.sqrt(a) +a +a = decimal.new('-123.456') +decimal.round(a, 2) +decimal.abs(a) a -a = decimal.tonumber('-123.456') -a:round(2) -a:round(1) -a:round(0) -a:abs() -a:tostring() -a a / 10 a * 5 a + 17 a - 0.0001 a ^ 2 -a:abs() ^ 0.5 -a:abs() ^ 0.5 == a:abs():sqrt() +decimal.abs(a) ^ 0.5 +decimal.abs(a) ^ 0.5 == decimal.sqrt(decimal.abs(a)) a - 2 < a - 1 a + 1e-10 > a a <= a a >= a a == a - -decimal.tostring(a) -a:tostring() -decimal.abs(a) -decimal.minus(a) -decimal.round(a, 2)