From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 24 Jul 2019 19:17:51 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH 4/5] decimal: add conversions to (u)int64_t
Message-ID: <20190724161751.GA24631@esperanza>
References: <cover.1563376957.git.sergepetrenko@tarantool.org>
 <bb69e149cf5f94463b4cc697c5f6643101870ebf.1563376957.git.sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bb69e149cf5f94463b4cc697c5f6643101870ebf.1563376957.git.sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Jul 17, 2019 at 06:33:45PM +0300, Serge Petrenko wrote:
> Update decNumber library, add methods to convert decimals to uint64_t
> and int64_t, add unit tests.
> Also replace decimal_round() function with decimal_round_with_mode() to
> allow setting rounding mode.

Better add decimal_floor(), as decimal_round_with_mode() has a
complicated API IMO.

> We need to round with mode DEC_ROUND_DOWN in to_int64 conversions in
> order to be consistent with double to int conversions.
> It will be needed to compute hints for decimal fields.
> 
> Prerequisite #4333
> ---
>  src/lib/core/decimal.c   | 63 ++++++++++++++++++++++++++++++++--
>  src/lib/core/decimal.h   | 12 +++++++
>  test/unit/decimal.c      | 66 ++++++++++++++++++++++++++++++++++-
>  test/unit/decimal.result | 74 ++++++++++++++++++++++++++++++++++++++--
>  third_party/decNumber    |  2 +-
>  5 files changed, 209 insertions(+), 8 deletions(-)
> 
> diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c
> index 2c69a773c..5a576e6e5 100644
> --- a/src/lib/core/decimal.c
> +++ b/src/lib/core/decimal.c
> @@ -157,6 +157,57 @@ decimal_to_string(const decimal_t *dec)
>  	return buf;
>  }
>  
> +static decimal_t *
> +decimal_round_with_mode(decimal_t *dec, int scale, enum rounding mode);
> +
> +decimal_t *
> +decimal_to_int64(decimal_t *dec, int64_t *num)

Why return decimal_t?

Shouldn't 'dec' be marked const?

> +{
> +	decimal_t d, z;
> +	decNumberZero(&z);
> +	d = *dec;
> +	dec = &d;

This looks confusing - like you're modifying the given decimal
number below.

> +
> +	if (decimal_scale(dec) != 0) {
> +		/*
> +		 * Rounding mode is important here.
> +		 * We want to be consistent with double
> +		 * to int conversion so that comparison
> +		 * hints work correctly.
> +		 */
> +		dec = decimal_round_with_mode(dec, 0, DEC_ROUND_DOWN);
> +	}
> +	/*
> +	 * decNumberToInt64 works only with numbers with
> +	 * zero exponent.
> +	 */
> +	decNumberRescale(dec, dec, &z, &decimal_context);
> +	if (decimal_check_status(dec, &decimal_context) == NULL) {
> +		return NULL;
> +	}
> +	*num = decNumberToInt64(dec, &decimal_context);
> +	return decimal_check_status(dec, &decimal_context);
> +}
> +
> +decimal_t *
> +decimal_to_uint64(decimal_t *dec, uint64_t *num)
> +{
> +	decimal_t d, z;
> +	decNumberZero(&z);
> +	d = *dec;
> +	dec = &d;
> +
> +	if (decimal_scale(dec) != 0) {
> +		dec = decimal_round_with_mode(dec, 0, DEC_ROUND_DOWN);
> +	}
> +	decNumberRescale(dec, dec, &z, &decimal_context);

It would be nice to factor out the piece of code shared by
decimal_to_uint64 and decimal_to_int64.

> +	if (decimal_check_status(dec, &decimal_context) == NULL) {
> +		return NULL;
> +	}
> +	*num = decNumberToUInt64(dec, &decimal_context);
> +	return decimal_check_status(dec, &decimal_context);
> +}
> +