From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 5 Jun 2019 15:40:14 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 4/4] decimal: add MessagePack encoding/decoding support Message-ID: <20190605124014.vylxrh6nfgoki2g4@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Serge Petrenko Cc: georgy@tarantool.org, tarantool-patches@freelists.org, kostja@tarantool.org List-ID: 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 > #include > > @@ -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.