Re: [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the fixes!

On 09.04.2021 23:08, Mergen Imeev via Tarantool-patches wrote:
> Thank you for the review! My answer and new patch below.
> 
> 
> On 30.03.2021 02:08, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> On 23.03.2021 10:36, Mergen Imeev via Tarantool-patches wrote:
>>> This patch introduces mem_get_unsigned() function which is used to
>>> receive unsigned value from MEM.
>>>
>>> Part of #5818
>>> ---
>>>  src/box/sql/func.c    | 16 +++++++++++-----
>>>  src/box/sql/mem.c     | 37 +++++++++++++++++++++++++++----------
>>>  src/box/sql/mem.h     |  6 +++---
>>>  src/box/sql/sqlInt.h  |  3 ---
>>>  src/box/sql/vdbeapi.c |  6 ------
>>>  5 files changed, 41 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>>> index 0fa0f6ac7..a851d98f2 100644
>>> --- a/src/box/sql/func.c
>>> +++ b/src/box/sql/func.c
>>> @@ -118,9 +118,12 @@ port_vdbemem_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
>>>  			luaL_pushint64(L, n);
>>>  			break;
>>>  		}
>>> -		case MP_UINT:
>>> -			luaL_pushuint64(L, sql_value_uint64(param));
>>> +		case MP_UINT: {
>>> +			uint64_t u;
>>> +			mem_get_unsigned(param, &u);
>>> +			luaL_pushuint64(L, u);
>> Maybe we could make 2 functions? One to get the value and ignore
>> the errors, and the other to get as an out parameter + return an
>> error?
>>
>> For instance, mem_to_uint() - returns uint64_t and internally asserts
>> that the value is correct. And mem_get_uint() works like your version.
>>
>> The same for the other get functions whose result is often ignored.
> For some functions I created a "proxy" functions in func.c the way you
> described, but not for this function since it is only used in a few places of
> sql/func.c. Should I do this for all functions? In func.c I mean. I see this as
> temporary measure, since I hope we will rework built-in functions one day.

Unfortunately, 'hope' is not enough. And it is highly possible the code
will live for long. Therefore I think we need to make it solid where possible
and clearly state it is unsafe or add assertions where it is not possible.

Here mem_get_uint() result is ignored always. Even if it fails. I think it
must be called something like mem_get_uint_unsafe() and return the uint as
'return', not via an out argument. Then at least we would see it is broken
when we are around this code again, and it won't raise questions if it is a
known issue, and why it is not fixed (this must be in a comment for the
function).