[Tarantool-patches] [PATCH v4 10/16] sql: refactor AVG() function

[Vladislav Shpilevoy]

Thanks for the fixes!

>> @@ -141,17 +141,14 @@ fin_avg(struct sql_context *ctx)
>>  	assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
>>  	if (mem_is_null(ctx->pMem))
>>  		return mem_set_null(ctx->pOut);
>> -	struct Mem *tmp = (struct Mem *)ctx->pMem->z;
>> -	uint32_t *count_val = (uint32_t *)(tmp + 1);
>> -	struct Mem sum;
>> -	mem_create(&sum);
>> -	mem_copy_as_ephemeral(&sum, tmp);
>> -	mem_destroy(tmp);
>> +	struct Mem *sum = (struct Mem *)ctx->pMem->z;
>> +	uint32_t *count_val = (uint32_t *)(sum + 1);
>>  	struct Mem count;
>>  	mem_create(&count);
>>  	mem_set_uint(&count, *count_val);
>>  	if (mem_div(&sum, &count, ctx->pOut) != 0)
>>  		ctx->is_aborted = true;
>> +	mem_destroy(sum);
>>  }
> This will work, however, I think it will create some unnecessary restrictions
> due to changes with pMem and pOut in a few patches. I suggest to apply part of
> you diff with exception of mem_destroy(), which I sugget to replace by assert().
> We have full control over this tmp/sum mem and we know, that there will be no
> memory to free, so assert should be enough.
> 
> What do you think of this diff?

It looks the same as mine except you didn't call the destroy. I am fine with it,
but when I propose to wrap the check about a mem not needing a destroy into a
function. We should not use mem members as is when possible. It is a too
complicated structure. So far.

Something like mem_is_trivial(). If it returns true, you don't need to
call mem_clear()/mem_destroy() and nothing will leak.