[tarantool-patches] [PATCH 2/2] decimal: expose decimal type to lua.
Serge Petrenko
sergepetrenko at tarantool.org
Fri Jun 28 17:39:39 MSK 2019
Hi! Thank you for review! I addressed your comments and sent v2.
The only inconsistency is with macro names: I feel like LDECIMAL_BINOP and LDECIMAL_FUNC
are better, since the latter one is only used for functions now (ln, log10, sqrt, abs).
--
Serge Petrenko
sergepetrenko at tarantool.org
> 26 июня 2019 г., в 17:02, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
>
> Pasting the whole patch here for review:
>
>> From 565aa457dfb40302797cdd1d9863559fe2c52468 Mon Sep 17 00:00:00 2001
>> From: Serge Petrenko <sergepetrenko at tarantool.org>
>> 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).
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190628/80b2a24a/attachment.html>
More information about the Tarantool-patches
mailing list