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 2533B23EBA for ; Fri, 1 Feb 2019 11:39:12 -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 e1PTy77ZefAA for ; Fri, 1 Feb 2019 11:39:12 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CFE62228DF for ; Fri, 1 Feb 2019 11:39:11 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code From: "n.pettik" In-Reply-To: <1487a209-af7b-5d1f-6fff-fc589782d571@tarantool.org> Date: Fri, 1 Feb 2019 19:39:10 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <239B8EBA-F577-4C9A-BDD9-D76DE10FEB76@tarantool.org> 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> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >>> I propose you to either come up with a better solution or to use one = of >>> my solutions: >>>=20 >>> * return AFFINITY_MASK as P5_FIELD_TYPE_MASK, and add a static = assertion >>> that P5_FIELD_TYPE_MASK < (1 << bit_count(field_type)). >>>=20 >>> Or write in field_def.h something like this: >>>=20 >>> static_assert(bit_count(field_type) <=3D 4, >>> "size of enum field_type is used in "\ >>> "VdbeOp.p5 to fit it into 4 bits=E2=80=9D); >> To be honest I don=E2=80=99t understand these bit assertions. >> Why can=E2=80=99t we simply check that field_type_MAX <=3D 15? >> diff --git a/src/box/field_def.c b/src/box/field_def.c >> index b34d2ccd9..1a8074650 100644 >> --- a/src/box/field_def.c >> +++ b/src/box/field_def.c >> @@ -33,6 +33,13 @@ >> #include "trivia/util.h" >> #include "key_def.h" >> +/** >> + * For detailed explanation see context of OP_Eq, OP_Lt etc >> + * opcodes in vdbe.c. >> + */ >> +static_assert(field_type_MAX <=3D 15, "values of enum field_type "\ >> + "should fit into 4 bits of VdbeOp.p5"); >> + >> Also, as you may notice, I placed it to field_def.c >> Seems that field_def.h is used to generate module.h, >> which in turn doesn=E2=80=99t tolerate this kind of assert. >=20 > field_def.h is not copied to module.h entirely. Only > the code between /** \cond public */ and /** \endcond public */. > Please, put this assertion below /** \endcond public */. Didn=E2=80=99t pay attention to that. Fixed: diff --git a/src/box/field_def.c b/src/box/field_def.c index 1a8074650..b34d2ccd9 100644 --- a/src/box/field_def.c +++ b/src/box/field_def.c @@ -33,13 +33,6 @@ #include "trivia/util.h" #include "key_def.h" =20 -/** - * For detailed explanation see context of OP_Eq, OP_Lt etc - * opcodes in vdbe.c. - */ -static_assert(field_type_MAX <=3D 15, "values of enum field_type "\ - "should fit into 4 bits of VdbeOp.p5"); - const char *mp_type_strs[] =3D { /* .MP_NIL =3D */ "nil", /* .MP_UINT =3D */ "unsigned", diff --git a/src/box/field_def.h b/src/box/field_def.h index 93e38ea55..358e35f69 100644 --- a/src/box/field_def.h +++ b/src/box/field_def.h @@ -87,6 +87,13 @@ affinity_type_str(enum affinity_type type); =20 /** \endcond public */ =20 +/** + * For detailed explanation see context of OP_Eq, OP_Lt etc + * opcodes in vdbe.c. + */ +static_assert(field_type_MAX <=3D 15, "values of enum field_type "\ + "should fit into 4 bits of VdbeOp.p5"); + extern const char *field_type_strs[]; =20 extern const char *on_conflict_action_strs[]; >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 80d2fd0aa..358b69754 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -2164,17 +2151,24 @@ case OP_Ge: { /* same as TK_GE, = jump, in1, in3 */ >> break; >> } >> } else { >> - /* Neither operand is NULL. Do a comparison. */ >> - affinity =3D pOp->p5 & AFFINITY_MASK; >> - if (affinity>=3DAFFINITY_INTEGER) { >> + /* >> + * Neither operand is NULL. Do a comparison. >> + * 15 is 1111 in a binary form. >> + * Since all existing types can be fitted in 4 bits >> + * (field_type_MAX =3D=3D 10), it is enough to 'and' >> + * p5 with this constant. Node that all other flags >> + * that can be stored in p5 are >=3D 16. >=20 > Firstly, as I said in the previous review, field_type_MAX =3D=3D 9. > Secondly, please, do not use a constant. All of them (15, 111, 10, = 16). > Instead of this huge comment full of constants just create a enum > value with an appropriate name, like FIELD_TYPE_MASK =3D 15, it is > even ok to put this name into field_def.h. Thirdly, not all p5 flags > are >=3D 16. But for these concrete opcodes it is so, luckily. At > least we have OPFLAG_NCHANGE, OPFLAG_EPHEM, OPFLAG_SEEKEQ etc - > they all are <=3D 15. Is this variant OK? +/** + * 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 =3D 15 +}; + extern const char *field_type_strs[]; =20 extern const char *on_conflict_action_strs[]; 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. - * 15 is 1111 in a binary form. - * Since all existing types can be fitted in 4 bits - * (field_type_MAX =3D=3D 10), it is enough to 'and' - * p5 with this constant. Node that all other flags - * that can be stored in p5 are >=3D 16. + * Since all existing types can be fitted in 4 + * bits (field_type_MAX =3D=3D 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 >=3D FIELD_TYPE_MASK. */ - enum field_type type =3D pOp->p5 & 15; + enum field_type type =3D pOp->p5 & FIELD_TYPE_MASK;