From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 14 Aug 2019 14:56:43 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 2/8] decimal: fix encoding numbers with positive exponent. Message-ID: <20190814115643.GF13834@esperanza> References: <53d58fa5cad01273d3e60fe034c8e74f023d3c0e.1565263272.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53d58fa5cad01273d3e60fe034c8e74f023d3c0e.1565263272.git.sergepetrenko@tarantool.org> To: Serge Petrenko Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: On Thu, Aug 08, 2019 at 02:55:53PM +0300, Serge Petrenko wrote: > When a number having a positive exponent is encoded, the internal > decPackedFromNumber function returns a negative scale, which differs > from the scale, returned by decimal_scale(). This leads to errors in > decoding. Account for negative scale in decimal_pack() and > decimal_unpack(). > > Follow-up #692 > --- > src/lib/core/decimal.c | 10 +++++++--- > test/unit/decimal.c | 15 +++++++++++++-- > test/unit/decimal.result | 11 ++++++++--- > 3 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c > index 6ef351f81..840aa5dfe 100644 > --- a/src/lib/core/decimal.c > +++ b/src/lib/core/decimal.c > @@ -33,6 +33,7 @@ > #include "third_party/decNumber/decContext.h" > #include "third_party/decNumber/decPacked.h" > #include "lib/core/tt_static.h" > +#include "lib/msgpuck/msgpuck.h" > #include > #include > #include /* DBL_DIG */ > @@ -340,12 +341,15 @@ char * > decimal_pack(char *data, const decimal_t *dec) > { > uint32_t len = decimal_len(dec); > - *data++ = decimal_scale(dec); > + /* reserve space for resulting scale */ > + char *svp = data++; > len--; > int32_t scale; > char *tmp = (char *)decPackedFromNumber((uint8_t *)data, len, &scale, dec); > assert(tmp == data); > - assert(scale == (int32_t)decimal_scale(dec)); > + /* scale may be negative, when exponent is > 0 */ > + assert(scale == (int32_t)decimal_scale(dec) || scale < 0); This assertion now looks kinda lopsided/incomplete and therefore confusing: we check positive scale but not negative scale. I'd rather remove it altogether. > + mp_store_u8(svp, (int8_t)scale); What is the reason to use mp_store/load helpers? AFAIU plain load/store should work just fine (as they did before this patch). Do I understand correctly that this particular patch must be committed to all stable branches as well?