Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. I added a comment about the
issue here:
https://github.com/tarantool/doc/issues/928#issuecomment-567449272

On Thu, Dec 19, 2019 at 12:42:10AM +0300, Igor Munkin wrote:
> Mergen,
> 
> Thanks, the changes are totally LGTM. However, please consider the side
> note I left below (I expect no changes in patch regarding it).
> 
> Cced Kirill to proceed with with the patch.
> 
> On 11.12.19, imeevma@tarantool.org wrote:
> 
> <snipped>
> 
> > 
> > New patch:
> > 
> > From 0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7 Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Fri, 6 Dec 2019 19:22:40 +0300
> > Subject: [PATCH] lua/serializer: encode Lua number -2^63 as integer
> > 
> > This patch fixes a bug that appeared after commit
> > 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: fix lua_tofield()
> > for 2**64 value') .
> > 
> > Due to this bug, -2^63 was serialized as double, although it
> > should be serialized as integer.
> > 
> > Closes #4672
> > 
> 
> <snipped>
> 
> > +
> > +    -- gh-4672: Make sure that -2^63 encoded as INTEGER.
> > +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
> > +            '-2^63 encoded as INTEGER')
> 
> Side note: After applying the following diff on your patch, tests are
> still fine.
> | diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
> | index 038559756..7d3f62109 100644
> | --- a/test/app-tap/lua/serializer_test.lua
> | +++ b/test/app-tap/lua/serializer_test.lua
> | @@ -145,7 +145,7 @@ local function test_signed(test, s)
> |      rt(test, s, ffi.new('int', -128), 'number')
> |
> |      -- gh-4672: Make sure that -2^63 encoded as INTEGER.
> | -    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
> | +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63 - 1),
> |              '-2^63 encoded as INTEGER')
> |  end
> Yep, it's double precision issue, and we can do nothing here, but I
> guess it ought to be well-documented, taking into account the patch
> you've made.
> 
> -- 
> Best regards,
> IM