From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 79CBD6B962; Wed, 14 Apr 2021 02:04:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 79CBD6B962 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618355048; bh=Ltv9I8UatQbwe0oOYG5c3rVOEw457tSZM7W89kBoSmg=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=vlF4khoVW139a244CiHBvxEoI5CTDgnVBGk2+Mr4G9nJRRwVUTN2uWJx1btCYFS1F 63WPKcifjhMkEYl5Tj/z7dLhGWZ16zyvU3tW4ouJ4ErWq/mB6/f63+OG90If0S3xAq rtmhUvTIbNOnOhTmtLTaNmnLELPXJtpvqKrXwzwA= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5B9996EC5B for ; Wed, 14 Apr 2021 02:04:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5B9996EC5B Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lWS4k-0002N8-Cs; Wed, 14 Apr 2021 02:04:06 +0300 To: imeevma@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: Message-ID: <29a16259-d26a-4391-4a8d-0439fec146a0@tarantool.org> Date: Wed, 14 Apr 2021 01:04:05 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480B1C8842CE613979723F2FB4628545A35182A05F538085040ACD1B78D7B491BADE1BEFAC61B82DEBD0FD72D762ED4E3959FCB56CADE14424C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7081BBE264C6D7F42EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375E280A1EC162AD7D8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2488B9FF3C3AF3DC9767211BD2ED0F8D68F4427C359EA06A0D2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE73AFA331E307B52169FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735D528F8CDFD49EC53C4224003CC83647689D4C264860C145E X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831A9F9CF5144222FFF2DAC2A4A94C307B6 X-C1DE0DAB: 0D63561A33F958A545408E234B83868A825CF7D42BE8D06B48CF0113B0F2CDC5D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34DFB7A809FB5370874132FEA160089739AD4880FD099A214B697CB28D87B4AD122C13E97F46001F921D7E09C32AA3244C58C609CEB51DAE05F9A81339B69DD91BB018FE5BB746DCD1FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXF2n5NNKLTiBw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822FE32C6DDAB12B2619D098082DCD289FE3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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).