[Tarantool-patches] [PATCH v4 10/16] sql: refactor AVG() function
v.shpilevoy at tarantool.org
Tue Oct 12 00:50:39 MSK 2021
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_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.
More information about the Tarantool-patches