Re: [tarantool-patches] [PATCH 2/2] decimal: expose decimal type to lua.

[Vladimir Davydov <vdavydov.dev@gmail.com>]

Pasting the whole patch here for review:

> From 565aa457dfb40302797cdd1d9863559fe2c52468 Mon Sep 17 00:00:00 2001
> From: Serge Petrenko <sergepetrenko@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).