[Tarantool-patches] [PATCH v4 12/53] sql: introduce mem_copy()

[Vladislav Shpilevoy]

I appreciate the work you did here!

See 2 comments below.

On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces mem_copy(). This function copies value from source
> MEM to destination MEM. In case value is string or binary and have not
> static allocation type, it is copied to newly allocated memory.
> 
> Part of #5818
> ---
>  src/box/sql/func.c    |  4 ++--
>  src/box/sql/mem.c     | 54 +++++++++++++++++++++++++------------------
>  src/box/sql/mem.h     |  9 +++++++-
>  src/box/sql/vdbeapi.c |  2 +-
>  src/box/sql/vdbeaux.c |  2 +-
>  5 files changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 81b537d9b..6b6081150 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -2079,13 +2079,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
>  		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
>  		cmp = sqlMemCompare(pBest, pArg, pColl);
>  		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
> -			sqlVdbeMemCopy(pBest, pArg);

1. Still is "used" in stat4ValueFromExpr().

> +			mem_copy(pBest, pArg);
>  		} else {
>  			sqlSkipAccumulatorLoad(context);
>  		}
>  	} else {
>  		pBest->db = sql_context_db_handle(context);
> -		sqlVdbeMemCopy(pBest, pArg);
> +		mem_copy(pBest, pArg);
>  	}
>  }
>  
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index abc9291ef..f12441d7c 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -257,6 +257,38 @@ mem_destroy(struct Mem *mem)
>  	mem->zMalloc = NULL;
>  }
>  
> +int
> +mem_copy(struct Mem *to, const struct Mem *from)
> +{
> +	mem_clear(to);
> +	to->u = from->u;
> +	to->flags = from->flags;
> +	to->subtype = from->subtype;
> +	to->field_type = from->field_type;
> +	to->n = from->n;
> +	to->z = from->z;
> +	if ((to->flags & (MEM_Str | MEM_Blob)) == 0)
> +		return 0;
> +	if ((to->flags & MEM_Static) != 0)
> +		return 0;
> +	if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob)) {
> +		if (sqlVdbeMemExpandBlob(to) != 0)
> +			return -1;
> +		return 0;

2. You can make `returnsqlVdbeMemExpandBlob(to);`, no need to check its result.

Also what was wrong with sqlVdbeMemCopy's way of using sqlVdbeMemMakeWriteable?

> +	}
> +	if (to->szMalloc == 0)
> +		to->zMalloc = sqlDbMallocRaw(to->db, to->n);
> +	else
> +		to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
> +	if (to->zMalloc == NULL)
> +		return -1;
> +	to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
> +	memcpy(to->zMalloc, to->z, to->n);
> +	to->z = to->zMalloc;
> +	to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype);
> +	return 0;
> +}
> +
>  static inline bool
>  mem_has_msgpack_subtype(struct Mem *mem)
>  {