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 8B33A6EC5B; Tue, 30 Mar 2021 02:05:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8B33A6EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617059119; bh=aWcWPng8R+VDkdynFAk0rcChhUlh6xW0UnkO+D5kPMU=; 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=F3UHm83q5Cqp78s+pRlQl+NyJs/V2yj3MNdPP3LoPU+Y+0FdZxE9NGtpEiBqUXFOH p/q7YCTMMExjEcZb+nk9j/gXpQQfYBTdz+9CKOyD/veuSHdAXVdUz8nZw67Ninkbmx y2VPELDgM4fdt9q/h8LM0LMjRqL4u7GCu1LaK1qE= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 31BD06BD06 for ; Tue, 30 Mar 2021 02:04:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 31BD06BD06 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lR0vn-00077F-AC; Tue, 30 Mar 2021 02:04:23 +0300 To: imeevma@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: Message-ID: Date: Tue, 30 Mar 2021 01:04:21 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 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: 4F1203BC0FB41BD9ED7173E37F4E32947A0146560F8BA709E798FFD99D1B1662182A05F53808504068BD9D5A7708AD26FCEF3274A1AFCC13C7DEE6CF1959E5254DF9C396E5F24165 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72267471453D8B600EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637525B22DCF689D4638638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CE99938B3FD79E1DF96F73CF927AC48E442A063788D7F19B9A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE7328B01A8D746D8839FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C353FA85A707D24CADCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CFC5EA940A35A165FF2DBA43225CD8A89FC0F9454058DFE53C57739F23D657EF2BB5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831B61D915BCC9FD1C9AB73A7D1547457A7 X-C1DE0DAB: 0D63561A33F958A55C0A8941A3F888543B7633F175D81FD3920EB1E1BF99DA87D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D343C45ADCD169245FADDC63788548592E9B97A1C5BAA13261B441105C7CDC181FF8C13D085D924870C1D7E09C32AA3244C69F3DB3F6778660AFB6D37D4DFC5E83B95A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojljIiQOC84rRE5krsMfrApg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822BC96687BFF7B4349E164E0FBB279CEDC3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v4 24/53] sql: introduce mem_set_integer() 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 patch! See 5 comments below. On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote: > This patch introduces mem_set_integer(). Function mem_set_integer() > clears MEM and sets it to given integer value. > > Part of #5818 > --- > src/box/sql/func.c | 6 +-- > src/box/sql/mem.c | 88 ++++++++++++++++++------------------------- > src/box/sql/mem.h | 21 ++++------- > src/box/sql/vdbe.c | 10 ++--- > src/box/sql/vdbeapi.c | 4 +- > src/box/sql/vdbeaux.c | 6 +-- > 6 files changed, 56 insertions(+), 79 deletions(-) > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index b61de18d8..8f7550f30 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -278,7 +278,7 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size) > mem_set_double(&val[i], field.dval); > break; > case MP_INT: > - mem_set_i64(&val[i], field.ival); > + mem_set_integer(&val[i], field.ival, true); 1. It is worth adding a function for setting a negative integer, like I mentioned in one of the previous emails. Might make such places easier to read. > break; > case MP_UINT: > mem_set_u64(&val[i], field.ival); > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index da2aa5c94..13a587aba 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -557,8 +566,7 @@ mem_arithmetic(const struct Mem *left, const struct Mem *right, > default: > unreachable(); > } > - result->u.i = ires; > - result->flags = is_res_neg ? MEM_Int : MEM_UInt; > + mem_set_integer(result, ires, is_res_neg); 2. mem_set_integer() calls mem_clear(), but you already called clear for the result in the beginning of this function. Better keep the old version here. Inside of mem functions you can do things more efficiently sometimes, without using the public API. Also there was no field_type set to FIELD_TYPE_INTEGER before. Why did you change that? It was NUMBER. > @@ -583,13 +592,13 @@ mem_bitwise(struct Mem *left, struct Mem *right, struct Mem *result, int op) > return -1; > } > if (op == OP_BitAnd) { > - result->u.i = l & r; > - result->flags = result->u.i < 0 ? MEM_Int : MEM_UInt; > + res = l & r; > + mem_set_integer(result, res, res < 0); 3. The same. Clear() is called second time and field_type is changed, but it wasn't before. Why? The same in some other similar places in the patch. > @@ -1359,15 +1368,14 @@ vdbe_mem_numerify(struct Mem *mem) > if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) != 0) > return 0; > if ((mem->flags & MEM_Bool) != 0) { > - mem->u.u = mem->u.b; > - MemSetTypeFlag(mem, MEM_UInt); > + mem_set_integer(mem, (int64_t)mem->u.b, false); 4. Why can't you replace it with mem_set_u64()? If this is because you need FIELD_TYPE_INTEGER, then see the question above why the field type is set now. > return 0; > }> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index f0b56033a..92845d66d 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2999,8 +2999,8 @@ case OP_FCopy: { /* out2 */ > assert(mem_is_integer(pIn1)); > > pOut = vdbe_prepare_null_out(p, pOp->p2); > - mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int); > - pOut->field_type = pIn1->field_type; > + if (mem_copy(pOut, pIn1) != 0) > + goto abort_due_to_error; 5. Why? It couldn't fail before, now it can. It copied just int before, now it calls the full copy function which looks like an overkill. > } > break; > }