[tarantool-patches] Re: [PATCH v2 8/8] box: get rid of box.internal.sql_function_create
Kirill Shcherbatov
kshcherbatov at tarantool.org
Fri Aug 16 15:57:14 MSK 2019
>> +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.
More information about the Tarantool-patches
mailing list