From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93])
 (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 0F6C546970E
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 21 Dec 2019 01:00:51 +0300 (MSK)
Received: by smtp33.i.mail.ru with esmtpa (envelope-from
 <v.shpilevoy@tarantool.org>) id 1iiQKJ-0007bX-F3
 for tarantool-patches@dev.tarantool.org; Sat, 21 Dec 2019 01:00:51 +0300
References: <20190912174448.25680-1-maria.khaydich@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <274febe4-c14a-e6b4-3dbd-6c8301a405ba@tarantool.org>
Date: Fri, 20 Dec 2019 23:00:50 +0100
MIME-Version: 1.0
In-Reply-To: <20190912174448.25680-1-maria.khaydich@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH]
 msgpackffi.decode can now be assigned to buf.rpos
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: tarantool-patches@dev.tarantool.org



Hi! Thanks for the patch!

Review of the first commit:

============================================================================

> Author: Maria <marianneliash@gmail.com>
>
>     lua: keeping the pointer type in msgpackffi.decode()
>     
>     Method decode_unchecked returns two values - the one that has
>     been decoded and a pointer to the new position within the buffer
>     given as a parameter. The type of returned pointer used to be
>     cdata<unsigned char *> and it was not possible to assign returned
>     value to buf.rpos due to the following error:
>     
>     > cannot convert 'const unsigned char *' to 'char *'
>     
>     The patch fixes this by making decode_unchecked method return either
>     cdata<char *> or cdata<const char *> depending on the given parameter.
>     
>     Closes #3926
>
> diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
> index 36ac26b7e..e64228e4d 100755
> --- a/test/app-tap/msgpackffi.test.lua
> +++ b/test/app-tap/msgpackffi.test.lua
> @@ -4,6 +4,8 @@ package.path = "lua/?.lua;"..package.path
>  
>  local tap = require('tap')
>  local common = require('serializer_test')
> +local buffer = require('buffer')

Buffer is never used here. Lets drop it.

> +local ffi = require('ffi')

============================================================================

Review of the second commit:

============================================================================

> Author: Alexander Turenko <alexander.turenko@tarantool.org>
>
>     lua: don't modify pointer type in msgpack.decode*
>     
>     msgpackffi.decode_unchecked([const] char *) returns two values: a
>     decoded result and a new pointer within passed buffer. After #3926 a
>     cdata type of the returned pointer follows a type of passed buffer.
>     
>     This commit modifies behaviour of msgpack module in the same way. The
>     following functions now returns cdata<char *> or cdata<const char *>
>     depending of its argument:
>     
>     * msgpack.decode(cdata<[const] char *>, number)
>     * msgpack.decode_unchecked(cdata<[const] char *>)
>     * msgpack.decode_array_header(cdata<[const] char *>, number)
>     * msgpack.decode_map_header(cdata<[const] char *>, number)
>     
>     Follows up #3926.
>
> diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
> index e64228e4d..be6906e67 100755
> --- a/test/app-tap/msgpackffi.test.lua
> +++ b/test/app-tap/msgpackffi.test.lua
> @@ -117,45 +117,6 @@ local function test_other(test, s)

Now "require('ffi')" in the beginning of that file becomes
not used. Please, drop it.