From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id DD78026938
	for <tarantool-patches@freelists.org>; Tue, 20 Aug 2019 15:36:52 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id 2Hd6n6r079MI for <tarantool-patches@freelists.org>;
	Tue, 20 Aug 2019 15:36:52 -0400 (EDT)
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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 761202610D
	for <tarantool-patches@freelists.org>; Tue, 20 Aug 2019 15:36:52 -0400 (EDT)
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: [tarantool-patches] Re: [PATCH v2 8/8] box: get rid of
 box.internal.sql_function_create
From: "n.pettik" <korablev@tarantool.org>
In-Reply-To: <9c0dfbfc-5cf2-5d9b-a7f4-f9e4140f6294@tarantool.org>
Date: Tue, 20 Aug 2019 22:36:46 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <9C2179FA-9019-46AB-8F6F-559BAFA5AA02@tarantool.org>
References: <cover.1565275469.git.kshcherbatov@tarantool.org>
 <8c42515c61f85e03dcd5abe35055b9301379de0a.1565275470.git.kshcherbatov@tarantool.org>
 <AE5CD1D0-5AB5-42FE-A299-A66BDFC303D9@tarantool.org>
 <9c0dfbfc-5cf2-5d9b-a7f4-f9e4140f6294@tarantool.org>
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-Help: <mailto:ecartis@freelists.org?Subject=help>
List-Unsubscribe: <mailto:tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-Subscribe: <mailto:tarantool-patches-request@freelists.org?Subject=subscribe>
List-Owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-Archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org
Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org>


>>> +static struct Mem *
>>> +vdbemem_alloc_on_region(uint32_t count)
>>=20
>> Nit: we already have sql_vdbe_mem_alloc_region()
>> which allocates memory for string using region.
>> Could you rename it (the original one I mean) to avoid
>> any confusions?
> Renamed to sql_vdbe_mem_alloc_blob_region

Please, move this fix to a separate patch.

>>> +static const char *
>>> +port_vdbemem_get_msgpack(struct port *base, uint32_t *size)
>>=20
>> In fact, this function is unused and is not tested at all.
> It is not so. Take a look to box/function1.test.lua

Oh, ok. Sorry, I=E2=80=99m so used to running only sql suite
that forget to look at other tests.

>>> + * Register P3 must not be one of the function inputs.
>>=20
>> Nit: you don=E2=80=99t check this fact.
> It is just recommendation for a user.

As a rule, users don=E2=80=99t even open source code.
This is solely developer oriented comment, so either place
assertion verifying that P3 doesn=E2=80=99t get into mentioned range
(if it is really vital) or remove this comment.

> Plz Forget about it.
> (moreover it *never* tested in other places)

Why should it be?

Also you skipped one nit:

-       if (pOut->flags & (MEM_Str|MEM_Blob))

->

+       if ((pOut->flags & (MEM_Str | MEM_Blob)) !=3D 0)
               if (sqlVdbeMemTooBig(pOut)) goto too_big;

Please, apply.

>> Oh, could you please fix this awful error message?
>> At least, it lacks a verb. Personally I would prefer smth like:
>> =E2=80=9Cinvalid number of arguments is passed to %s: expected %d, =
got %d"
> Fixed as a separate patch.

Look at fixes at np/sql-builtins
I=E2=80=99ve introduced separate error code for invalid argument count.