From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <imeevma@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 44A1646970E
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 19 Dec 2019 15:34:13 +0300 (MSK)
Date: Thu, 19 Dec 2019 15:34:10 +0300
From: Mergen Imeev <imeevma@tarantool.org>
Message-ID: <20191219123410.GA21765@tarantool.org>
References: <0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7.1576060409.git.imeevma@gmail.com>
 <20191218214210.GX1214@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191218214210.GX1214@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua
 number -2^63 as integer
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.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