[tarantool-patches] Re: [PATCH v2 8/8] box: get rid of box.internal.sql_function_create

[n.pettik]

>>> +static struct Mem *
>>> +vdbemem_alloc_on_region(uint32_t count)
>> 
>> 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)
>> 
>> 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’m so used to running only sql suite
that forget to look at other tests.

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

As a rule, users don’t even open source code.
This is solely developer oriented comment, so either place
assertion verifying that P3 doesn’t 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)) != 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:
>> “invalid number of arguments is passed to %s: expected %d, got %d"
> Fixed as a separate patch.

Look at fixes at np/sql-builtins
I’ve introduced separate error code for invalid argument count.