[Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion

[Mergen Imeev]

Hi! Thank you the review! My answer below.

On Fri, Oct 29, 2021 at 12:11:11AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> >>> +	assert(mem_is_int(arg));
> >>> +	uint64_t u = mem_is_uint(arg) ? arg->u.u : (uint64_t)-arg->u.i;
> >>
> >> 2. You could make return when mem_is_uint(). It would remove '?' and
> >> mem_set_uint() which would calls mem_clear() inside.
> >>
> > I am not sure that I understood correctly. In case of argument being uint we
> > can use mem_copy_as_ephemeral() instead of mem_set_uint(), but I am not sure
> > if it would be better.
> 
> I mean this:
> 
> ====================
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index dbeb38bee..2a848be31 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -239,11 +239,10 @@ func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv)
>  	assert(argc == 1);
>  	(void)argc;
>  	struct Mem *arg = &argv[0];
> -	if (mem_is_null(arg))
> +	if (mem_is_null(arg) || mem_is_uint(arg))
>  		return;
>  	assert(mem_is_int(arg));
> -	uint64_t u = mem_is_uint(arg) ? arg->u.u : (uint64_t)-arg->u.i;
> -	mem_set_uint(ctx->pOut, u);
> +	mem_set_uint(ctx->pOut, (uint64_t)-arg->u.i);
>  }
> ====================
> 
> Up to you.
This would work for aggregate function in some cases, but not here.
If we apply such diff the result of ABS() for UNSIGNED will be NULL.