From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5BFC526741 for ; Tue, 5 Feb 2019 10:08:19 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rN37RMvJglmP for ; Tue, 5 Feb 2019 10:08:19 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0E06925239 for ; Tue, 5 Feb 2019 10:08:18 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code References: <541AF1EA-6688-4B4F-92CB-2AA15765A62E@tarantool.org> <2cd04a2d-6cd8-9e0b-7d51-d9431b6c5d82@tarantool.org> <33150EBA-AB14-4089-8399-0FBD07C71BD8@tarantool.org> <1487a209-af7b-5d1f-6fff-fc589782d571@tarantool.org> <239B8EBA-F577-4C9A-BDD9-D76DE10FEB76@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 5 Feb 2019 18:08:16 +0300 MIME-Version: 1.0 In-Reply-To: <239B8EBA-F577-4C9A-BDD9-D76DE10FEB76@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 358b69754..dae05e80f 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2153,13 +2153,13 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > } else { > /* > * Neither operand is NULL. Do a comparison. 'Neither operand is NULL' - this is 'else' branch of "(flags1 | flags3)&MEM_Null", so I guess it is obvious. > - * 15 is 1111 in a binary form. > - * Since all existing types can be fitted in 4 bits > - * (field_type_MAX == 10), it is enough to 'and' > - * p5 with this constant. Node that all other flags > - * that can be stored in p5 are >= 16. > + * Since all existing types can be fitted in 4 > + * bits (field_type_MAX == 9), it is enough to > + * 'and' p5 with field mask. Note that values of > + * other flags that can be stored in p5 of these > + * opcodes are >= FIELD_TYPE_MASK. They can not be == FIELD_TYPE_MASK. Please, again, do not try to write here any constants, including bit counts. A definition named MASK is already enough to make it clear, that p5 'and' mask give a type. > */ > - enum field_type type = pOp->p5 & 15; > + enum field_type type = pOp->p5 & FIELD_TYPE_MASK; > Please, apply the diff below. I made enum field_type_mask an anonymous enum, as we usually do with independent constants. Also, I used FIELD_TYPE_MASK instead of 15 in static assertion, and removed the comment from vdbe.c about what a mask is. diff --git a/src/box/field_def.h b/src/box/field_def.h index 3b8eb0ccf..3eda0e38f 100644 --- a/src/box/field_def.h +++ b/src/box/field_def.h @@ -87,21 +87,22 @@ affinity_type_str(enum affinity_type type); /** \endcond public */ +enum { + /** + * This mask allows to store in VdbeOp.p5 operand of + * OP_Eq, OP_Lt etc opcodes field type alonside with + * flags. + */ + FIELD_TYPE_MASK = 15 +}; + /** * For detailed explanation see context of OP_Eq, OP_Lt etc * opcodes in vdbe.c. */ -static_assert(field_type_MAX <= 15, "values of enum field_type "\ +static_assert(field_type_MAX <= FIELD_TYPE_MASK, "values of enum field_type "\ "should fit into 4 bits of VdbeOp.p5"); -/** - * This mask allows to store in p5 operand of OP_Eq, OP_Lt etc - * opcodes field type alonside with other flags. - */ -enum field_type_mask { - FIELD_TYPE_MASK = 15 -}; - extern const char *field_type_strs[]; extern const char *on_conflict_action_strs[]; diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index dae05e80f..a66ab4992 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2151,14 +2151,6 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ break; } } else { - /* - * Neither operand is NULL. Do a comparison. - * Since all existing types can be fitted in 4 - * bits (field_type_MAX == 9), it is enough to - * 'and' p5 with field mask. Note that values of - * other flags that can be stored in p5 of these - * opcodes are >= FIELD_TYPE_MASK. - */ enum field_type type = pOp->p5 & FIELD_TYPE_MASK; if (sql_type_is_numeric(type)) { if ((flags1 | flags3)&MEM_Str) {