Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: georgy@tarantool.org, tarantool-patches@freelists.org,
	kostja@tarantool.org
Subject: Fwd: Re: [PATCH v3 4/4] decimal: add MessagePack encoding/decoding support
Date: Thu, 6 Jun 2019 12:27:58 +0300	[thread overview]
Message-ID: <20190606092757.otvjaigijmx72wjd@esperanza> (raw)

[ RESEND ]

On Tue, Jun 04, 2019 at 07:04:19PM +0300, Serge Petrenko wrote:
> Add methods to encode/decode decimals as msgpack.
> 
> Part of #692
> ---
>  cmake/BuildDecNumber.cmake   |   1 +
>  src/lib/core/decimal.c       |  64 +++++++++++
>  src/lib/core/decimal.h       |  38 +++++++
>  src/lib/core/mp_user_types.h |  38 +++++++
>  test/unit/decimal.c          |  73 ++++++++++++-
>  test/unit/decimal.result     | 204 ++++++++++++++++++++++++++++++++++-
>  6 files changed, 416 insertions(+), 2 deletions(-)
>  create mode 100644 src/lib/core/mp_user_types.h
> 
> diff --git a/cmake/BuildDecNumber.cmake b/cmake/BuildDecNumber.cmake
> index 80942fe05..abc6c64c4 100644
> --- a/cmake/BuildDecNumber.cmake
> +++ b/cmake/BuildDecNumber.cmake
> @@ -4,6 +4,7 @@ macro(decnumber_build)
>      set(decnumber_src
>  	${PROJECT_SOURCE_DIR}/third_party/decNumber/decNumber.c
>  	${PROJECT_SOURCE_DIR}/third_party/decNumber/decContext.c
> +	${PROJECT_SOURCE_DIR}/third_party/decNumber/decPacked.c
>      )
>  
>      add_library(decNumber STATIC ${decnumber_src})
> diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c
> index 12250da95..7b3b1d8c9 100644
> --- a/src/lib/core/decimal.c
> +++ b/src/lib/core/decimal.c
> @@ -30,7 +30,10 @@
>   */
>  
>  #include "decimal.h"
> +#include "mp_user_types.h"
> +#include "src/lib/msgpuck/msgpuck.h"
>  #include "third_party/decNumber/decContext.h"
> +#include "third_party/decNumber/decPacked.h"
>  #include <stdlib.h>
>  #include <assert.h>
>  
> @@ -291,3 +294,64 @@ decimal_sqrt(decimal *res, const decimal *lhs, uint32_t precision)
>  
>  	return decimal_finalize(res, &op_context);
>  }
> +
> +static inline uint32_t
> +decimal_len(decimal *dec)
> +{
> +	       /* 1 + ceil((digits + 1) / 2) */

Bad indentation.

> +	return 2 + dec->digits / 2;
> +}
> +
> +uint32_t
> +mp_sizeof_decimal(decimal *dec) {
> +	uint32_t l = decimal_len(dec);
> +	return mp_sizeof_extl(l) + l;

Better use mp_sizeof_ext instead of mp_sizeof_extl here?

> +}
> +
> +char *
> +mp_encode_decimal(char *data, decimal *dec)
> +{
> +	uint32_t len = decimal_len(dec);
> +	data = mp_encode_extl(data, MP_DECIMAL, len);
> +	data = mp_store_u8(data, decimal_scale(dec));
> +	len--;
> +	int32_t scale;
> +	char *tmp = (char *)decPackedFromNumber((uint8_t *)data, len, &scale, dec);
> +	assert(tmp == data);
> +	assert(scale == (int32_t)decimal_scale(dec));
> +	data += len;
> +	return data;
> +}
> +
> +decimal *
> +mp_decode_decimal_raw(const char **data, decimal *dec, uint32_t len)
> +{
> +	int32_t scale = mp_load_u8(data);
> +	len--;
> +	decimal *res = decPackedToNumber((uint8_t *)*data, len, &scale, dec);
> +	if (res)
> +		*data += len;
> +	else
> +		(*data)--;
> +	return res;
> +}
> +
> +decimal *
> +mp_decode_decimal(const char **data, decimal *dec)
> +{
> +	int8_t type;
> +	uint32_t len;
> +
> +	if (mp_typeof(**data) != MP_EXT)
> +		return NULL;
> +	const char *const svp = *data;
> +	len = mp_decode_extl(data, &type);
> +
> +	if (type != MP_DECIMAL || len == 0)
> +		return NULL;
> +	decimal *res = mp_decode_decimal_raw(data, dec, len);
> +	if (!res) {
> +		*data = svp;
> +	}
> +	return res;
> +}
> diff --git a/src/lib/core/decimal.h b/src/lib/core/decimal.h
> index f2812f500..cfcc98073 100644
> --- a/src/lib/core/decimal.h
> +++ b/src/lib/core/decimal.h
> @@ -156,4 +156,42 @@ decimal_exp(decimal *res, const decimal *lhs, uint32_t precision);
>  decimal *
>  decimal_sqrt(decimal *res, const decimal *lhs, uint32_t precision);
>  
> +
> +/**
> + * Return length of message pack encoded decimal with extension header.
> + * Equivalent to mp_sizeof_ext(decimal representation length)
> + */
> +uint32_t
> +mp_sizeof_decimal(decimal *dec);
> +
> +/**
> + * Encode a decimal value to msgpack.
> + *
> + * @return data + mp_sizeof_decimal(dec)
> + */
> +char *
> +mp_encode_decimal(char *data, decimal *dec);
> +
> +/**
> + * Decode a decimal value as though msgpack
> + * ext header was already decoded.
> + *
> + * \post *data = *data + value representation length
> + * @return NULL if value encoding is incorrect
> + *	    dec  otherwise.
> + */
> +decimal *
> +mp_decode_decimal_raw(const char **data, decimal *dec, uint32_t len);
> +
> +/**
> + * Decode a decimal encoded as msgpack with ext header.
> + *
> + * \post *data = *data + mp_sizeof_decimal(dec)
> + * @return NULL if mp_typeof(**data) != MP_EXT
> + *		   or value encoding is incorrect
> + *	   dec  otherwise.
> + */
> +decimal *
> +mp_decode_decimal(const char **data, decimal *dec);
> +

Mixing mp_ and decimal_ methods in the same file looks ugly, because
msgpack is just one format - we might add other formats in future and it
doesn't sound like a good idea to implement their decoders in decimal.c.
IMO decimal.h should only contain methods to pack/unpack decimals in
some format, say

  decimal_pack_len/decimal_pack/decimal_unpack

or

  decimal_encode/decimal_decode

mp_ methods should live in some other file (mp_ext, mp_ext_decimal,
mp_decimal.[hc]?)

And I think it's better add and test those generic methods in patch 2,
along with the decimal infrastructure while this patch should be about
msgpack ext only.

             reply	other threads:[~2019-06-06  9:27 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06  9:27 Vladimir Davydov [this message]
2019-06-11 16:04 ` [tarantool-patches] " Serge Petrenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190606092757.otvjaigijmx72wjd@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=georgy@tarantool.org \
    --cc=kostja@tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Fwd: Re: [PATCH v3 4/4] decimal: add MessagePack encoding/decoding support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox