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 51DEF6EC5D; Fri, 9 Apr 2021 21:13:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 51DEF6EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617991994; bh=f5hT/cXhZHnh7RbuKKX6O/4EoqQuXuyGdYky/zEd4DQ=; h=To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=PixFGbB2RNvNZ9k2XHAfv30bRa4DcUHeyoucJ51p9dHN54EDLeSOXp/7ODqOzGgpT ac1LyXaMr8EqrxgArjWo+qCOSdNyz6Vcq4bZnhbcHkhPNdOyh3R8Y1EetW6Mgdr1yS 9yowsSUKQu4Sjf0ETHGFRwREZckbqiqIH5W1Eye8= 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 38E706EC63 for ; Fri, 9 Apr 2021 21:11:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 38E706EC63 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lUvbe-0004Ia-Cd; Fri, 09 Apr 2021 21:11:46 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 21:11:46 +0300 Message-Id: <69aa47b1575b768da04cc579e70038f5be272027.1617984948.git.imeevma@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480D608FE24BC85426BB1B55F651FED8C70182A05F53808504057EAC787FAA43155240D44503E631E78957229D26FEF660025EA97016EB93B2F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73D5736A43F78B50EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375C4806A08D329A618638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B270A9CABF7C5D0F8061EC06EE6594F01079373A8315E4CF65D2E47CDBA5A96583C09775C1D3CA48CF776A0366D588B3C3117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE749E2213E709ACCBA9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735E4A630A5B664A4FFC4224003CC83647689D4C264860C145E X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4F348FA7728E70B838786E79604D660A549C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340DAE5B306C240CF5873E69933DF313D06E26625E92031EC3E1B0B1457B70E7726EF28404943579D61D7E09C32AA3244CEB6F37A749F00CF992B9AFB8F8C467FAD9ADFF0C0BDB8D1FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4TkTCV3a13vdw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822E18B2D2BE1B552424EFDBF9B345780F483D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH v5 24/52] sql: introduce mem_set_int() 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: Mergen Imeev via Tarantool-patches Reply-To: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers and new patch below. On 30.03.2021 02:04, Vladislav Shpilevoy wrote: > 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. > I moved this block of code to mem.c and inlined mem setting. >> 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. > Thank, fixed. I inlined mem_set_*() where we do not need clear(). > Also there was no field_type set to FIELD_TYPE_INTEGER before. Why > did you change that? It was NUMBER. > Fixed. >> @@ -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. > Fixed. Inlined. >> @@ -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. > Fixed. Inlined. >> 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. > Replaced mem_copy() to mem_copy_as_ephemeral(). In current case mem_copy() and mem_copy_as_ephemeral() are not different since they both should just copy a few fields. However, mem_copy_as_ephemeral() cannot return an error. >> } >> break; >> } New patch: commit 69aa47b1575b768da04cc579e70038f5be272027 Author: Mergen Imeev Date: Mon Mar 15 11:29:40 2021 +0300 sql: introduce mem_set_int() This patch introduces mem_set_int(). This function clears MEM and sets it to given integer value. Part of #5818 diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 937fa3272..075000218 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -273,6 +273,15 @@ mem_set_null(struct Mem *mem) mem_clear(mem); } +void +mem_set_int(struct Mem *mem, int64_t value, bool is_neg) +{ + mem_clear(mem); + mem->u.i = value; + mem->flags = is_neg ? MEM_Int : MEM_UInt; + mem->field_type = FIELD_TYPE_INTEGER; +} + int mem_copy(struct Mem *to, const struct Mem *from) { @@ -1384,8 +1393,9 @@ 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->u.u = (uint64_t)mem->u.b; + mem->flags = MEM_UInt; + mem->field_type = FIELD_TYPE_UNSIGNED; return 0; } assert((mem->flags & (MEM_Blob | MEM_Str)) != 0); @@ -1454,20 +1464,23 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type) return 0; } if ((pMem->flags & MEM_Bool) != 0) { - pMem->u.u = pMem->u.b; - MemSetTypeFlag(pMem, MEM_UInt); + pMem->u.u = (uint64_t)pMem->u.b; + pMem->flags = MEM_UInt; + pMem->field_type = FIELD_TYPE_UNSIGNED; return 0; } if ((pMem->flags & MEM_Real) != 0) { - double d; - if (sqlVdbeRealValue(pMem, &d) != 0) - return -1; - if (d < (double)INT64_MAX && d >= (double)INT64_MIN) { - mem_set_int(pMem, d, d <= -1); + double d = pMem->u.r; + if (d < 0. && d >= (double)INT64_MIN) { + pMem->u.i = (int64_t)d; + pMem->flags = MEM_Int; + pMem->field_type = FIELD_TYPE_INTEGER; return 0; } - if (d >= (double)INT64_MAX && d < (double)UINT64_MAX) { - mem_set_u64(pMem, d); + if (d >= 0. && d < (double)UINT64_MAX) { + pMem->u.u = (uint64_t)d; + pMem->flags = MEM_UInt; + pMem->field_type = FIELD_TYPE_UNSIGNED; return 0; } return -1; @@ -1518,8 +1531,11 @@ mem_apply_integer_type(Mem *pMem) assert(pMem->flags & MEM_Real); assert(EIGHT_BYTE_ALIGNMENT(pMem)); - if ((rc = doubleToInt64(pMem->u.r, (int64_t *) &ix)) == 0) - mem_set_int(pMem, ix, pMem->u.r <= -1); + if ((rc = doubleToInt64(pMem->u.r, (int64_t *) &ix)) == 0) { + pMem->u.i = ix; + pMem->flags = pMem->u.r <= -1 ? MEM_Int : MEM_UInt; + pMem->field_type = FIELD_TYPE_INTEGER; + } return rc; } @@ -1663,13 +1679,20 @@ mem_apply_type(struct Mem *record, enum field_type type) if (double_compare_uint64(d, UINT64_MAX, 1) > 0) return 0; - if ((double)(uint64_t)d == d) - mem_set_u64(record, (uint64_t)d); + if ((double)(uint64_t)d == d) { + record->u.u = (uint64_t)d; + record->flags = MEM_UInt; + record->field_type = + FIELD_TYPE_UNSIGNED; + } } else { if (double_compare_nint64(d, INT64_MIN, 1) < 0) return 0; - if ((double)(int64_t)d == d) - mem_set_int(record, (int64_t)d, true); + if ((double)(int64_t)d == d) { + record->u.i = (int64_t)d; + record->flags = MEM_Int; + record->field_type = FIELD_TYPE_INTEGER; + } } return 0; } @@ -1781,7 +1804,9 @@ mem_convert_to_unsigned(struct Mem *mem) double d = mem->u.r; if (d < 0.0 || d >= (double)UINT64_MAX) return -1; - mem_set_u64(mem, (uint64_t) d); + mem->u.u = (uint64_t)d; + mem->flags = MEM_UInt; + mem->field_type = FIELD_TYPE_UNSIGNED; return 0; } @@ -1801,10 +1826,15 @@ mem_convert_to_integer(struct Mem *mem) double d = mem->u.r; if (d >= (double)UINT64_MAX || d < (double)INT64_MIN) return -1; - if (d < (double)INT64_MAX) - mem_set_int(mem, (int64_t) d, d < 0); - else - mem_set_int(mem, (uint64_t) d, false); + if (d < 0.) { + mem->u.i = (int64_t)d; + mem->flags = MEM_Int; + mem->field_type = FIELD_TYPE_INTEGER; + } else { + mem->u.u = (uint64_t)d; + mem->flags = MEM_UInt; + mem->field_type = FIELD_TYPE_UNSIGNED; + } return 0; } @@ -1923,16 +1953,6 @@ mem_set_ptr(struct Mem *mem, void *ptr) mem->u.p = ptr; } -void -mem_set_i64(struct Mem *mem, int64_t value) -{ - mem_clear(mem); - mem->u.i = value; - int flag = value < 0 ? MEM_Int : MEM_UInt; - MemSetTypeFlag(mem, flag); - mem->field_type = FIELD_TYPE_INTEGER; -} - void mem_set_u64(struct Mem *mem, uint64_t value) { @@ -1942,21 +1962,6 @@ mem_set_u64(struct Mem *mem, uint64_t value) mem->field_type = FIELD_TYPE_UNSIGNED; } -void -mem_set_int(struct Mem *mem, int64_t value, bool is_neg) -{ - mem_clear(mem); - if (is_neg) { - assert(value < 0); - mem->u.i = value; - MemSetTypeFlag(mem, MEM_Int); - } else { - mem->u.u = value; - MemSetTypeFlag(mem, MEM_UInt); - } - mem->field_type = FIELD_TYPE_INTEGER; -} - void mem_set_double(struct Mem *mem, double value) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 581ef007a..788f3c89f 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -166,6 +166,10 @@ mem_destroy(struct Mem *mem); void mem_set_null(struct Mem *mem); +/** Clear MEM and set it to INTEGER. */ +void +mem_set_int(struct Mem *mem, int64_t value, bool is_neg); + /** * Copy content of MEM from one MEM to another. In case source MEM contains * string or binary and allocation type is not STATIC, this value is copied to @@ -447,24 +451,10 @@ mem_set_bool(struct Mem *mem, bool value); void mem_set_ptr(struct Mem *mem, void *ptr); -/** - * Set integer value. Depending on its sign MEM_Int (in case - * of negative value) or MEM_UInt flag is set. - */ -void -mem_set_i64(struct Mem *mem, int64_t value); - /** Set unsigned value and MEM_UInt flag. */ void mem_set_u64(struct Mem *mem, uint64_t value); -/** - * Set integer value. According to is_neg flag value is considered - * to be signed or unsigned. - */ -void -mem_set_int(struct Mem *mem, int64_t value, bool is_neg); - /** Set double value and MEM_Real flag. */ void mem_set_double(struct Mem *mem, double value); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index c7d21ddd5..0a3de8ac9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -763,7 +763,7 @@ case OP_Halt: { */ case OP_Integer: { /* out2 */ pOut = vdbe_prepare_null_out(p, pOp->p2); - mem_set_i64(pOut, pOp->p1); + mem_set_int(pOut, pOp->p1, pOp->p1 < 0); break; } @@ -3090,8 +3090,7 @@ case OP_FCopy: { /* out2 */ assert(mem_is_int(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; + mem_copy_as_ephemeral(pOut, pIn1); } break; } diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 4398b41b1..737477285 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -189,7 +189,7 @@ sql_result_uint(sql_context *ctx, uint64_t u_val) void sql_result_int(sql_context *ctx, int64_t val) { - mem_set_i64(ctx->pOut, val); + mem_set_int(ctx->pOut, val, val < 0); } void diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index e0d815958..8e50d47bf 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1324,13 +1324,13 @@ sqlVdbeList(Vdbe * p) } } - mem_set_i64(pMem, pOp->p1); + mem_set_int(pMem, pOp->p1, pOp->p1 < 0); pMem++; - mem_set_i64(pMem, pOp->p2); + mem_set_int(pMem, pOp->p2, pOp->p2 < 0); pMem++; - mem_set_i64(pMem, pOp->p3); + mem_set_int(pMem, pOp->p3, pOp->p3 < 0); pMem++; if (sqlVdbeMemClearAndResize(pMem, 256)) {