Fwd: Re: [PATCH v3 4/4] decimal: add MessagePack encoding/decoding support

Vladimir Davydov vdavydov.dev at gmail.com
Thu Jun 6 12:27:58 MSK 2019


[ 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.



More information about the Tarantool-patches mailing list