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@tarantool.org




> 26 июня 2019 г., в 17:02, 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).
>