Re: [Tarantool-patches] [PATCH v5 28/52] sql: introduce mem_copy_str() and mem_copy_str0()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the fixes!

See 2 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 87e1bcfd1..045c44e8f 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -394,6 +394,34 @@ mem_set_str0_allocated(struct Mem *mem, char *value)
>  	mem->flags |= MEM_Term;
>  }
>  
> +int
> +mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
> +{
> +	if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
> +		mem_clear(mem);

1. Why don't you call clear always? Anyway 'dynamic' memory can't
be reused. It is freed in sqlVdbeMemGrow() AFAIS.

> +	bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
> +			    mem->z == value;
> +	if (sqlVdbeMemGrow(mem, len, is_own_value) != 0)
> +		return -1;
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index e0903e3b0..ee095f36e 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -798,8 +803,14 @@ bindText(sql_stmt * pStmt,	/* The statement to bind against */
>  			mem_set_str0_dynamic(pVar, (char *)zData);
>  		else
>  			mem_set_str_dynamic(pVar, (char *)zData, nData);
> -	} else if (sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel) != 0) {
> -		return -1;
> +	} else {
> +		if (nData < 0) {
> +			if (mem_copy_str0(pVar, zData) != 0)
> +				return -1;
> +		} else {
> +			if (mem_copy_str(pVar, zData, nData) != 0)
> +				return -1;
> +		}

2. For mem_set_str* and mem_copy_str* you could probably have 3
versions: mem_set_strn(const char *, uint32 len),
mem_set_str0(const char *), and mem_set_str(const char *, uint32 len_hint).
In the last version len_hint might be -1 and then strlen() is called.
It would keep the places like that as simple as they were.

>  	}
>  	return sql_bind_type(p, i, "text");
>  }
>