[Tarantool-patches] [PATCH v4 30/53] sql: introduce mem_set_*_binary()

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 59a378e1b..5ee49cdca 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -410,6 +410,61 @@ mem_copy_string0(struct Mem *mem, const char *value)
>  	return 0;
>  }
>  
> +static inline void
> +mem_set_const_bin(struct Mem *mem, char *value, uint32_t size, int alloc_type)
> +{
> +	assert((alloc_type & (MEM_Static | MEM_Ephem)) != 0);
> +	mem_clear(mem);
> +	mem->z = value;
> +	mem->n = size;
> +	mem->flags = MEM_Blob | alloc_type;
> +	mem->field_type = FIELD_TYPE_VARBINARY;
> +}
> +
> +static inline void
> +mem_set_dyn_bin(struct Mem *mem, char *value, uint32_t size, int alloc_type)
> +{
> +	assert((mem->flags & MEM_Dyn) == 0 || value != mem->z);
> +	assert(mem->szMalloc == 0 || value != mem->zMalloc);
> +	assert(alloc_type == MEM_Dyn || alloc_type == 0);
> +	mem_destroy(mem);

1. Why is it destroy here and clear above?

> +	mem->z = value;
> +	mem->n = size;
> +	mem->flags = MEM_Blob | alloc_type;
> +	mem->field_type = FIELD_TYPE_VARBINARY;
> +	if (alloc_type == MEM_Dyn) {
> +		mem->xDel = sql_free;
> +	} else {
> +		mem->xDel = NULL;
> +		mem->zMalloc = mem->z;
> +		mem->szMalloc = sqlDbMallocSize(mem->db, mem->zMalloc);
> +	}
> +}
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 5b5e5b0c8..0e51e4809 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -183,7 +182,13 @@ sql_result_blob(sql_context * pCtx,
>      )
>  {
>  	assert(n >= 0);
> -	if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 0, xDel) != 0)
> +	if (xDel == SQL_STATIC)
> +		mem_set_static_binary(pCtx->pOut, (char *)z, n);
> +	else if (xDel == SQL_DYNAMIC)
> +		mem_set_allocated_binary(pCtx->pOut, (char *)z, n);
> +	else if (xDel != SQL_TRANSIENT)
> +		mem_set_dynamic_binary(pCtx->pOut, (char *)z, n);
> +	else if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 0, xDel) != 0)
>  		pCtx->is_aborted = true;

2. It seems to me you need to add a generic mem_set_binary which would
take the xdel argument. Repeating this tree of ifs in each usage place
is not any better. The same for the string API.