From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 26 Jun 2019 17:02:53 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH 2/2] decimal: expose decimal type to lua. Message-ID: <20190626140253.qzeog6z3rlopm7m4@esperanza> References: <89c41adbdf7452c1ef4a8f478363fe2ccb95a848.1560958964.git.sergepetrenko@tarantool.org> <20190621155307.z6pnpfbgqgni6zud@esperanza> <977F1A26-1374-4D64-8717-AB67F41E298F@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <977F1A26-1374-4D64-8717-AB67F41E298F@tarantool.org> To: Serge Petrenko , g@esperanza Cc: tarantool-patches@freelists.org List-ID: Pasting the whole patch here for review: > From 565aa457dfb40302797cdd1d9863559fe2c52468 Mon Sep 17 00:00:00 2001 > From: Serge Petrenko > Date: Wed, 5 Jun 2019 15:58:49 +0300 > Subject: [PATCH] decimal: expose decimal type to lua. > > 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 `new` 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.new(123e-7) > b = decimal.new('123.456') > c = decimal.new('123.456e2') > d = decimal.new(123ULL) > e = decimal.new(2) > ``` > 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`: There's no decimal.add anymore. > > Operations only fail on an overflow, i.e. when result exceeds 10^38 - 1. > This includes division by zero. In these cases an error `Operation > failed` is raised. > Underflow is also possible, when precision needed to store the exact > result exceeds 38 digits. Underflow is not an error. When an underflow > happens, the result is rounded to 38 digits of precision. > > ``` > tarantool> a + b > --- > - '123.456012300000000' > ... > > tarantool> c - d > --- > - '12222.6' > ... > > tarantool> c / b > --- > - '100' > ... > > tarantool> d * d > --- > - '15129' > ... > > tarantool> d ^ 2 > --- > - '15129' > ... > > tarantool> 2 ^ d > --- > - '10633823966279326983230456482242756608' > ... > > tarantool> e ^ d > --- > - '10633823966279326983230456482242756608' > ... > ``` It's unclear what a, b, c, d, e are in this example. > 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.new(100) > > tarantool> decimal.log10(f) > --- > - '2' > ... > > tarantool> decimal.sqrt(f) > --- > - '10' > ... > > tarantool> e2 = decimal.exp(2) > --- > ... > > tarantool> decimal.ln(e2) > --- > - '2.0000000000000000000000000000000000000' > ... > > There are also `abs` and `tostring` methods, and an unary minus > operator, which are pretty self-explanatory. > > ``` > tarantool> a = decimal.new(-5) > --- > ... > > tarantool> a > --- > - '-5.000000000000000' > ... This looks weird. Can't we trim trailing zeros somehow? > > tarantool> decimal.abs(a) > --- > - '5.000000000000000' > ... > > tarantool> -a > --- > - '5.000000000000000' > ... > > tostring(a) > --- > - '-5.000000000000000' > ... > > ``` > > `decimal.precision`, `decimal.scale` and `decimal.round` : > The first two methods return precision, i.e. decimal digits in > number representation, and scale, i.e. decimal digits after the decimal > point in the number representation. > `decimal.round` rounds the number to the given scale. > ``` > tarantool> a = decimal.new('123.456789') > --- > ... > > tarantool> decimal.precision(a) > --- > - 9 > ... > > tarantool> decimal.scale(a) > --- > - 6 > ... > > tarantool> decimal.round(a, 4) > --- > - '123.4568' > ... > > ``` > > Comparsions: `>`, `<`, `>=`, `<=`, `==` are also legal and work as > expected. You may compare decimals with lua numbers or strings. In that > case comparsion will happen after the values are converted to decimal > type. > > diff --git a/src/lua/decimal.c b/src/lua/decimal.c > new file mode 100644 > index 00000000..4dc0dff7 > --- /dev/null > +++ b/src/lua/decimal.c > @@ -0,0 +1,335 @@ > +#define LDECIMAL_OP2(name, opname)\ > +static int \ > +ldecimal_##name(struct lua_State *L) {\ > + if (lua_gettop(L) < 2)\ > + return luaL_error(L, "Usage: decimal."#name"(decimal, decimal)");\ There's no decimal_mul or decimal_add anymore... > + decimal_t *lhs = lua_todecimal(L, 1);\ > + decimal_t *rhs = lua_todecimal(L, 2);\ > + decimal_t *res = lua_pushdecimal(L);\ > + if (decimal_##opname(res, lhs, rhs) == NULL) {\ > + lua_pop(L, 1);\ > + luaL_error(L, "Operation failed.");\ The error message isn't informative. Should be "Decimal overflow" I think. > + }\ > + return 1;\ > +} Please align backslash (\): #define LDECIMAL_OP2(name, opname) \ static int \ ldecimal_##name(struct luaState *L) \ and so on. Also, please add brief comments to all helper functions. > +#define LDECIMAL_UNOP(name, opname)\ I'd call them LDECIMAL_OP1 and LDECIMAL_OP2 or LDECIMAL_UNOP and LDECIMAL_BINOP, for consistency. > +static int \ > +ldecimal_##name(struct lua_State *L) {\ > + if (lua_gettop(L) < 1)\ > + return luaL_error(L, "Usage: decimal."#name"(decimal)");\ There's no decimal.minus. > + decimal_t *lhs = lua_todecimal(L, 1);\ > + decimal_t *res = lua_pushdecimal(L);\ > + if (decimal_##opname(res, lhs) == NULL) {\ > + lua_pop(L, 1);\ > + luaL_error(L, "Operation failed");\ > + }\ > + return 1;\ > +} > + > +#define LDECIMAL_CMPOP(name, cmp)\ > +static int \ > +ldecimal_##name(struct lua_State *L) {\ > + if (lua_gettop(L) < 2)\ > + return luaL_error(L, "Usage: decimal.__"#name"(decimal, decimal)");\ > + decimal_t *lhs = lua_todecimal(L, 1);\ > + decimal_t *rhs = lua_todecimal(L, 2);\ > + lua_pushboolean(L, decimal_compare(lhs, rhs) cmp 0);\ > + return 1;\ > +} > + > +uint32_t CTID_DECIMAL; Should be static. > + > +static decimal_t * > +lua_pushdecimal(struct lua_State *L) > +{ > + decimal_t *res = luaL_pushcdata(L, CTID_DECIMAL); > + return res; > +} > + > +static decimal_t * > +lua_checkdecimal(struct lua_State *L, int index) > +{ > + uint32_t ctypeid; > + decimal_t *res = luaL_checkcdata(L, index, &ctypeid); > + if (ctypeid != CTID_DECIMAL) > + luaL_error(L, "Expected decimal as %d argument", index); > + return res; > +} > + > +static decimal_t * > +lua_todecimal(struct lua_State *L, int index) > +{ > + /* > + * Convert the index, if it is given relative to the top. > + * Othervise it will point to a wrong position after > + * pushdecimal(). > + */ > + if (index < 0) > + index = lua_gettop(L) + index + 1; > + decimal_t *res = lua_pushdecimal(L); > + switch(lua_type(L, index)) > + { > + case LUA_TNUMBER: > + { > + double n = lua_tonumber(L, index); > + if (decimal_from_double(res, n) == NULL) > + goto err; > + break; > + } > + case LUA_TSTRING: > + { > + const char *str = lua_tostring(L, index); > + if (decimal_from_string(res, str) == NULL) > + goto err; > + break; > + } > + case LUA_TCDATA: > + { > + uint32_t ctypeid; > + void *cdata = luaL_checkcdata(L, index, &ctypeid); > + int64_t ival; > + uint64_t uval; > + double d; > + if (ctypeid == CTID_DECIMAL) { > + /* > + * We already have a decimal at the > + * desired position. > + */ > + lua_pop(L, 1); > + return (decimal_t *) cdata; > + } > + switch (ctypeid) > + { > + case CTID_CCHAR: > + case CTID_INT8: > + ival = *(int8_t *) cdata; > + if (decimal_from_int64(res, ival) == NULL) > + goto err; > + break; > + case CTID_INT16: > + ival = *(int16_t *) cdata; > + if (decimal_from_int64(res, ival) == NULL) > + goto err; > + break; > + case CTID_INT32: > + ival = *(int32_t *) cdata; > + if (decimal_from_int64(res, ival) == NULL) > + goto err; > + break; > + case CTID_INT64: > + ival = *(int64_t *) cdata; > + if (decimal_from_int64(res, ival) == NULL) > + goto err; > + break; > + case CTID_UINT8: > + uval = *(uint8_t *) cdata; > + if (decimal_from_uint64(res, uval) == NULL) > + goto err; > + break; > + case CTID_UINT16: > + uval = *(uint16_t *) cdata; > + if (decimal_from_uint64(res, uval) == NULL) > + goto err; > + break; > + case CTID_UINT32: > + uval = *(uint32_t *) cdata; > + if (decimal_from_uint64(res, uval) == NULL) > + goto err; > + break; > + case CTID_UINT64: > + uval = *(uint64_t *) cdata; > + if (decimal_from_uint64(res, uval) == NULL) > + goto err; > + break; > + case CTID_FLOAT: > + d = *(float *) cdata; > + if (decimal_from_double(res, d) == NULL) > + goto err; > + break; > + case CTID_DOUBLE: > + d = *(double *) cdata; > + if (decimal_from_double(res, d) == NULL) > + goto err; > + break; > + default: > + lua_pop(L, 1); > + luaL_error(L, "expected decimal, number or string as %d argument", index); > + } > + break; > + } > + default: > + lua_pop(L, 1); > + luaL_error(L, "expected decimal, number or string as %d argument", index); > + } > + lua_replace(L, index); > + return res; > +err: /* pop the decimal we prepared on top of the stack. */ > + lua_pop(L, 1); > + luaL_error(L, "Incorrect value to convert to decimal as %d argument", index); Too long lines. Please split. Also, sometimes you start an error message with a lower-case letter, sometimes with an upper-case. Please be consistent. > + /* luaL_error never returns, this is to silence compiler warning. */ > + return NULL; > +} > + > +LDECIMAL_OP2(add, add); > +LDECIMAL_OP2(sub, sub); > +LDECIMAL_OP2(mul, mul); > +LDECIMAL_OP2(div, div); > +LDECIMAL_OP2(pow, pow); > + > +LDECIMAL_UNOP(log10, log10); > +LDECIMAL_UNOP(ln, ln); > +LDECIMAL_UNOP(exp, exp); > +LDECIMAL_UNOP(sqrt, sqrt); > +LDECIMAL_UNOP(minus, minus); > +LDECIMAL_UNOP(abs, abs); > + > +LDECIMAL_CMPOP(eq, ==); > +LDECIMAL_CMPOP(lt, <); > +LDECIMAL_CMPOP(le, <=); Semicolons (;) are not needed here, as these macros define functions. > + > +static int > +ldecimal_new(struct lua_State *L) > +{ > + if (lua_gettop(L) < 1) > + luaL_error(L, "Usage: decimal.new(value)"); > + lua_todecimal(L, 1); > + decimal_t *lhs = lua_checkdecimal(L, 1); Why not simply decimal_t lhs = lua_todecimal(L, 1); ? > + decimal_t *res = lua_pushdecimal(L); > + *res = *lhs; > + return 1; > +} > diff --git a/src/lua/decimal.h b/src/lua/decimal.h > new file mode 100644 > index 00000000..12b29d39 > --- /dev/null > +++ b/src/lua/decimal.h > @@ -0,0 +1,39 @@ > +#ifndef TARANTOOL_LUA_DECIMAL_H_INCLUDED > +#define TARANTOOL_LUA_DECIMAL_H_INCLUDED You forgot to add extern "C". > + > +struct lua_State; > + > +void > +tarantool_lua_decimal_init(struct lua_State *L); > + > +#endif /* TARANTOOL_LUA_DECIMAL_H_INCLUDED */ > diff --git a/test/app/decimal.test.lua b/test/app/decimal.test.lua > new file mode 100644 > index 00000000..b101f07b > --- /dev/null > +++ b/test/app/decimal.test.lua > @@ -0,0 +1,42 @@ > +decimal = require('decimal') > +test_run = require('test_run').new() > + > +-- check various constructors > +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.new('100') > +decimal.log10(a) > +decimal.ln(a) > +-- overflow, e^100 > 10^38 > +decimal.exp(a) > +-- e^87 < 10^38, no overflow, but rounds > +-- to 0 digits after the decimal point. > +decimal.exp(decimal.new(87)) > +decimal.sqrt(a) > +a > +a = decimal.new('-123.456') > +decimal.round(a, 2) > +decimal.abs(a) > +a > +-a > +a / 10 > +a * 5 > +a + 17 > +a - 0.0001 > +a ^ 2 > +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 Please add more tests: - All possible errors thrown by the code checking arguments. - All overflow errors, resulting from both unary and binary operators. - Creating a decimal from all kinds of objects (UINT/INT/DOUBLE...) In other words, I want to see that every single line of you code is covered: https://coveralls.io/builds/24193215/source?filename=src/lua/decimal.c Also, it might be worth checking that none of the functions occasionally modifies the decimal passed to it (i.e. it does create a new object).