[tarantool-patches] [PATCH 2/2] decimal: expose decimal type to lua.
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Jun 26 17:02:53 MSK 2019
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).
More information about the Tarantool-patches
mailing list