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 7FE136FCBF; Sun, 25 Apr 2021 20:41:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7FE136FCBF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619372513; bh=ZySCQvJGW7Tr25jZ4pRChXwWhqQfPGwhCVD8r1/drHk=; 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=FZq/s8hyMZQS44lVPtqGVdmzfF/+DMB7Q+4gUpLrmRuBP+mLS+54iLJb5kxsSPuJg XuYtx5hE0vqIXOtIq2ZzWXt7sKBLLDPzy5RO9t2QJhz5bmoG1f1/CO9TvbZFmnUzYH Kr1PRQaICTEuSmj88re+aQWJfLe7b2Y6rJ6SSOIQ= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 1DEF96FC8B for ; Sun, 25 Apr 2021 20:41:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1DEF96FC8B Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lailS-0003x5-4E; Sun, 25 Apr 2021 20:41:50 +0300 To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <9f3ee5e450d2e133bae1e0c131246b1f113f5608.1619182541.git.imeevma@gmail.com> Message-ID: <08572433-e7af-8960-499a-7aff8ab06286@tarantool.org> Date: Sun, 25 Apr 2021 19:41:48 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <9f3ee5e450d2e133bae1e0c131246b1f113f5608.1619182541.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9203E2ABA940B75486323DEBBB34FA34686FAAF51BCED67EE182A05F5380850406DFBFEA9CF2F3C0239F01E778C79E5FDD770F5ADCC9D4A135895D9B211DB9B17 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77545ECFDF1E157EBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637389D8DDD54F43F7A8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2F68949FFDB188B00BA685FFCE38F28FAEABCB1FA743BAAB4D2E47CDBA5A96583C09775C1D3CA48CFE97D2AE7161E217F117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71DD432BB81541BCF9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16BCE5475246E174218B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A521C7B37CA91DE8702CF470A5E067B71EDAA22908A1AB1E74D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D041FB2E16F174C4BCBC35721C180E1BD2D15B1785578AC53204C6781C4C514B69C34AA16BCD436C1D7E09C32AA3244C0675F2F09F21AC64FCB3A9E96A2C91B9F522A1CF68F4BE05729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj08M52wfuxcHJiA4H2cVutA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638222579903A8C2F28E98A19F1D611F418113841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/1] sql: replace MEM-type flags by enum mem_type 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" Hi! Thanks for the patch! See 15 comments below. Most of them are the same though. > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 9c28d5122..2ce7a3871 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -1766,14 +1766,20 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) > > struct func_sql_builtin *func = > (struct func_sql_builtin *)context->func; > + /* > + * TODO: Make proper initialization for aggregate accumulation > + * structures. Currently a hack is applied here: in case mem pBest is a > + * newly created MEM, it bytes are set to 0 using memset(). Since > + * MEM_NULL == 0, it works. However, it does not look right. > + */ 1. Not sure I understand. I see sql_aggregate_context() calls mem_set_agg() which sets MEM_AGG. Isn't it a proper initialization? > pBest = (Mem *) sql_aggregate_context(context, sizeof(*pBest)); > if (!pBest) > return; > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index b6ff6397f..90883ee21 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -60,26 +60,26 @@ const char * > mem_str(const struct Mem *mem) > { > char buf[BUF_SIZE]; > - switch (mem->flags & MEM_PURE_TYPE_MASK) { > - case MEM_Null: > + switch (mem->type) { > + case MEM_NULL: 2. Better use MEM_TYPE_*. Otherwise in case the mem will have any other enums, the value names would confuse. It is already so because the flags also have MEM_ prefix and the only difference is they are not uppercase in the suffix. > @@ -149,7 +148,8 @@ 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->type = is_neg ? MEM_INT : MEM_UINT; > + mem->flags = 0; 3. The flags are already cleared by mem_clear(). The same in mem_set_uint(), mem_set_bool(), mem_set_double(). > @@ -281,7 +287,8 @@ mem_copy_str(struct Mem *mem, const char *value, uint32_t len) > return -1; > memcpy(mem->z, value, len); > mem->n = len; > - mem->flags = MEM_Str; > + mem->type = MEM_STR; > + mem->flags = 0; 4. Ditto. The same in mem_copy_bin(). > mem->field_type = FIELD_TYPE_STRING; > return 0; > } > @@ -459,14 +471,16 @@ void > mem_set_invalid(struct Mem *mem) > { > mem_clear(mem); > - mem->flags = MEM_Undefined; > + mem->type = MEM_INVALID; > + mem->flags = 0; 5. The same as above + mem_set_ptr(), mem_set_frame(), mem_set_agg(). > @@ -498,19 +514,21 @@ void > mem_set_null_clear(struct Mem *mem) > { > mem_clear(mem); > - mem->flags = MEM_Null | MEM_Cleared; > + mem->flags = MEM_Cleared; > } > > static inline int > int_to_double(struct Mem *mem) > { > + assert(mem->type == MEM_INT || mem->type == MEM_UINT); > double d; > - if ((mem->flags & MEM_UInt) != 0) > + if (mem->type == MEM_UINT) > d = (double)mem->u.u; > else > d = (double)mem->u.i; > mem->u.r = d; > - mem->flags = MEM_Real; > + mem->type = MEM_DOUBLE; > + mem->flags = 0; 6. Do you need to touch the flags? If it was an int, its flags should have been 0 already. The same in int_to_bool(), double_to_int(), double_to_int_precise(), double_to_uint(), double_to_uint_precise(), double_to_bool(), bool_to_int(). > @@ -550,8 +571,9 @@ str_to_str0(struct Mem *mem) > static inline int > str_to_bin(struct Mem *mem) > { > - mem->flags = (mem->flags & (MEM_Dyn | MEM_Static | MEM_Ephem)) | > - MEM_Blob; > + assert(mem->type == MEM_STR); > + mem->type = MEM_BIN; > + mem->flags = mem->flags & (MEM_Dyn | MEM_Static | MEM_Ephem); 7. You can now use &=. But did you check if you even need to touch these flags? It seems you can leave them as is. Previously they were reset just to drop MEM_Blob flag I think. The same in bin_to_str(). > mem->field_type = FIELD_TYPE_VARBINARY; > return 0; > }> @@ -1168,9 +1229,9 @@ mem_get_bin(const struct Mem *mem, const char **s) > int > mem_len(const struct Mem *mem, uint32_t *len) > { > - if ((mem->flags & (MEM_Str | MEM_Blob)) == 0) > + if (mem->type != MEM_STR && mem->type != MEM_BIN) > return -1; 8. Is it -1 for MAP and ARRAY intentionally? Previously they were included into MEM_Blob. I see mem_concat() didn't check for subtypes. Does it mean we could concat two MP_ARRAYs into something invalid? If we could, your patch probably just fixed it. Could you check and add a test if so? > - if ((mem->flags & MEM_Blob) !=0 && (mem->flags & MEM_Zero) != 0) > + if (mem->type == MEM_BIN && (mem->flags & MEM_Zero) != 0) > *len = mem->n + mem->u.nZero; > else > *len = mem->n; > @@ -1236,7 +1298,8 @@ mem_move(struct Mem *to, struct Mem *from) > { > mem_destroy(to); > memcpy(to, from, sizeof(*to)); > - from->flags = MEM_Null; > + from->type = MEM_NULL; > + from->flags = 0; 9. Flags are already 0 after mem_destroy(). The same in mem_add(), mem_sub(), mem_mul(), mem_div(), mem_rem(), mem_bit_and(), mem_bit_or(), mem_shift_left(), mem_shift_right(), mem_bit_not(). > @@ -2208,7 +2264,8 @@ sqlValueNew(sql * db) > { > Mem *p = sqlDbMallocZero(db, sizeof(*p)); > if (p) { > - p->flags = MEM_Null; > + p->type = MEM_NULL; > + p->flags = 0; 10. MallocZero nullifies the memory, no need to set the flags to 0. > p->db = db; > } > return p; > @@ -2223,7 +2280,8 @@ releaseMemArray(Mem * p, int N) > assert((&p[1]) == pEnd || p[0].db == p[1].db); > assert(sqlVdbeCheckMemInvariants(p)); > mem_destroy(p); > - p->flags = MEM_Undefined; > + p->type = MEM_INVALID; > + p->flags = 0; 11. mem_destroy() already cleared the flags. > } while ((++p) < pEnd); > } > } > @@ -2844,23 +2919,28 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size) > mem_clear(&val[i]); > switch (field.type) { > case MP_BOOL: > - val[i].flags = MEM_Bool; > + val[i].type = MEM_BOOL; > + val[i].flags = 0; 12. The flags are cleared by mem_clear() above. The same in the other places in this function. The same in port_c_get_vdbemem(). > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index 1db7f4deb..48c1850d4 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -37,6 +37,35 @@ struct region; > struct mpstream; > struct VdbeFrame; > > +enum mem_type { > + MEM_NULL = 0, > + MEM_UINT, > + MEM_INT, > + MEM_STR, > + MEM_BIN, > + MEM_ARRAY, > + MEM_MAP, > + MEM_BOOL, > + MEM_FLOAT, > + MEM_DOUBLE, > + MEM_INVALID, > + MEM_FRAME, > + MEM_PTR, > + MEM_AGG, > +}; > + > +static_assert((int)MP_NIL == (int)MEM_NULL); > +static_assert((int)MP_UINT == (int)MEM_UINT); > +static_assert((int)MP_INT == (int)MEM_INT); > +static_assert((int)MP_STR == (int)MEM_STR); > +static_assert((int)MP_BIN == (int)MEM_BIN); > +static_assert((int)MP_ARRAY == (int)MEM_ARRAY); > +static_assert((int)MP_MAP == (int)MEM_MAP); > +static_assert((int)MP_BOOL == (int)MEM_BOOL); > +static_assert((int)MP_FLOAT == (int)MEM_FLOAT); > +static_assert((int)MP_DOUBLE == (int)MEM_DOUBLE); > +static_assert((int)MP_EXT == (int)MEM_INVALID); 13. You could also assign them right inside of enum declaration. Then wouldn't need to assert. > + > /* > * Internally, the vdbe manipulates nearly all SQL values as Mem > * structures. Each Mem struct may cache multiple representations (string, > @@ -57,9 +86,8 @@ struct Mem { > struct func *func; > struct VdbeFrame *pFrame; /* Used when flags==MEM_Frame */ > } u; > - u32 flags; /* Some combination of MEM_Null, MEM_Str, MEM_Dyn, etc. */ > - /** Subtype for this value. */ > - enum sql_subtype subtype; > + enum mem_type type; > + u32 flags; /* Some combination of MEM_Term, MEM_Zero, MEM_Dyn, etc. */ 14. Since you changed the line, it is better to put the comment on top of the member, according to the code style. And add a comment to the type field. Even a trivial one would work. Comments between members help to make the struct look more 'structured'. > @@ -114,175 +120,153 @@ struct Mem { <...> > static inline bool > mem_is_agg(const struct Mem *mem) > { > - return (mem->flags & MEM_Agg) != 0; > + return mem->type == MEM_AGG; > } > > static inline bool > mem_is_bytes(const struct Mem *mem) > { > - return (mem->flags & (MEM_Blob | MEM_Str)) != 0; > + enum mem_type type = mem->type; > + return type == MEM_BIN || type == MEM_MAP || type == MEM_ARRAY || > + type == MEM_STR; > } 15. It was good when we could check several rare cases at one branch. Maybe you could preserve the bitwise structure of the types? Then we could keep the bit operations and save a few branches. You didn't think of it, or do we go for normal numbers intentionally? On the other hand, having the types equal to mp types wins when need to convert mem_type to mp_type, but do we need it often?