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

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

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.