[Tarantool-patches] [PATCH v4 24/53] sql: introduce mem_set_integer()

[Vladislav Shpilevoy]

Thanks for the patch!

See 5 comments below.

On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces mem_set_integer(). Function mem_set_integer()
> clears MEM and sets it to given integer value.
> 
> Part of #5818
> ---
>  src/box/sql/func.c    |  6 +--
>  src/box/sql/mem.c     | 88 ++++++++++++++++++-------------------------
>  src/box/sql/mem.h     | 21 ++++-------
>  src/box/sql/vdbe.c    | 10 ++---
>  src/box/sql/vdbeapi.c |  4 +-
>  src/box/sql/vdbeaux.c |  6 +--
>  6 files changed, 56 insertions(+), 79 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index b61de18d8..8f7550f30 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -278,7 +278,7 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
>  			mem_set_double(&val[i], field.dval);
>  			break;
>  		case MP_INT:
> -			mem_set_i64(&val[i], field.ival);
> +			mem_set_integer(&val[i], field.ival, true);

1. It is worth adding a function for setting a negative integer,
like I mentioned in one of the previous emails. Might make such
places easier to read.

>  			break;
>  		case MP_UINT:
>  			mem_set_u64(&val[i], field.ival);
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index da2aa5c94..13a587aba 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -557,8 +566,7 @@ mem_arithmetic(const struct Mem *left, const struct Mem *right,
>  	default:
>  		unreachable();
>  	}
> -	result->u.i = ires;
> -	result->flags = is_res_neg ? MEM_Int : MEM_UInt;
> +	mem_set_integer(result, ires, is_res_neg);

2. mem_set_integer() calls mem_clear(), but you already called
clear for the result in the beginning of this function. Better
keep the old version here. Inside of mem functions you can do
things more efficiently sometimes, without using the public API.

Also there was no field_type set to FIELD_TYPE_INTEGER before. Why
did you change that? It was NUMBER.

> @@ -583,13 +592,13 @@ mem_bitwise(struct Mem *left, struct Mem *right, struct Mem *result, int op)
>  		return -1;
>  	}
>  	if (op == OP_BitAnd) {
> -		result->u.i = l & r;
> -		result->flags = result->u.i < 0 ? MEM_Int : MEM_UInt;
> +		res = l & r;
> +		mem_set_integer(result, res, res < 0);

3. The same. Clear() is called second time and field_type is changed,
but it wasn't before. Why? The same in some other similar places in the
patch.

> @@ -1359,15 +1368,14 @@ vdbe_mem_numerify(struct Mem *mem)
>  	if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) != 0)
>  		return 0;
>  	if ((mem->flags & MEM_Bool) != 0) {
> -		mem->u.u = mem->u.b;
> -		MemSetTypeFlag(mem, MEM_UInt);
> +		mem_set_integer(mem, (int64_t)mem->u.b, false);

4. Why can't you replace it with mem_set_u64()? If this is because you
need FIELD_TYPE_INTEGER, then see the question above why the field
type is set now.

>  		return 0;
>  	}> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index f0b56033a..92845d66d 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2999,8 +2999,8 @@ case OP_FCopy: {     /* out2 */
>  		assert(mem_is_integer(pIn1));
>  
>  		pOut = vdbe_prepare_null_out(p, pOp->p2);
> -		mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int);
> -		pOut->field_type = pIn1->field_type;
> +		if (mem_copy(pOut, pIn1) != 0)
> +			goto abort_due_to_error;

5. Why? It couldn't fail before, now it can. It copied just
int before, now it calls the full copy function which looks
like an overkill.

>  	}
>  	break;
>  }