Re: [PATCH v2 2/8] decimal: fix encoding numbers with positive exponent.

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

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 <stddef.h>
>  #include <stdlib.h>
>  #include <float.h> /* 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?