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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

>> +struct sql_context;
> 
> Nit: forward declaration of sql_context is redundant.
>> +    uint32_t size;
> 
> Nit: I’d better call it mem_count (or at least add brief explanation comment).
Fixed.
> +	 * allocated on the fiber()->gc, in which case the caller
>> +	 * is responsible for cleaning up.
>> +	 */
> 
> Consider comment fixes:
Applied.

>> +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

>> +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

>> +			mpstream_encode_str(&stream,
>> +					(const char *) sql_value_text(param));
> 
> Nit: a bit broken indentation. You can use auxiliary tmp var.
Fixed.

>> + * Register P3 must not be one of the function inputs.
> 
> Nit: you don’t check this fact.
It is just recommendation for a user. Plz Forget about it.
(moreover it *never* tested in other places)
> 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.