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 80B2721812 for ; Tue, 10 Sep 2019 19:14:13 -0400 (EDT) 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 ZNoewXBP7-gX for ; Tue, 10 Sep 2019 19:14:13 -0400 (EDT) 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 AEB0021005 for ; Tue, 10 Sep 2019 19:14:12 -0400 (EDT) Date: Wed, 11 Sep 2019 02:14:09 +0300 From: Nikita Pettik Subject: [tarantool-patches] Re: [PATCH 2/2] Group assertions and move them out of functions Message-ID: <20190910231409.GB27128@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Alexey Zavodchenkov On Fri, Aug 30, 2019 at 06:48:01PM +0300, Alexey Zavodchenkov wrote: Hi. Sorry for quite delayed answer. Firstly, don't put your fixes at a separate commit: just amend in case of commit (git commit --amend) or squash in case of interactive rebase. Then, if you send next patch version bump version passing --subject-prefix='PATCH v2' option to format-patch. Or, you can simply answer to the review letter with provided fixes (i.e. simple inline diff relared to each reviewer's comment). Read details here: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review Below a few codestyle nits and general comments. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 668562896..de356d29b 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -3669,6 +3669,31 @@ exprCodeVector(Parse * pParse, Expr * p, int *piFreeable) > return iResult; > } > > +/* > + * static asserts for sqlExprCodeTarget, sqlExprIfTrue and sqlExprIfFalse > + */ Nit: comments in global scope start from double stars: /** * Comment starts from capital letter and ends with a dot. It must not * exceed 72 characters per line. What is more, we don't write comments * just for the sake of commenting - it should explain something. In * this particular case I'd deliver this information: * * Make sure that opcode and token definitions are placed in the correct * order: value of opcode representing unary or binary operation must * be equal to values of corresponding opcode implementing that * operation. It allows to pass token value to sqlVdbeAddOp*() functions * as an opcode value. It is quite convinient since we can avoid mapping * tokens to opcodes. */ Intead of copy-pasting the same string several times, I'd rather once declare it and re-use (and a bit clarify error message): #define TOKEN_TO_OPCODE_ERR "Token representing binary or unary operator "\ "is not equal to opcode implementing it" static_assert(TK_LT == OP_Lt, TOKEN_TO_OPCODE_ERR); Or you can even use stringizing of macro arguments: #define TOKEN_TO_OPCODE_ERR(token, opcode) "Token "#token" representing ... Then we will get fine error message: /tarantool/src/box/sql/expr.c:497:1: error: static_assert failed "Token TK_LT representing binary or unary operator is not equal to opcode OP_Lt implementing it" static_assert(TK_GT == OP_Lt, TOKEN_TO_OPCODE_ERR(TK_LT, OP_Lt)); I demonstrated you example of good comment to this part of asserts. Please, investigate why do we need other asserts and write the same *good* comments for the other two groups. > +static_assert(TK_LT == OP_Lt, "inconsistent token/opcode definition"); > +static_assert(TK_LE == OP_Le, "inconsistent token/opcode definition"); > +static_assert(TK_GT == OP_Gt, "inconsistent token/opcode definition"); > +static_assert(TK_GE == OP_Ge, "inconsistent token/opcode definition"); > +static_assert(TK_EQ == OP_Eq, "inconsistent token/opcode definition"); > +static_assert(TK_NE == OP_Ne, "inconsistent token/opcode definition"); > +static_assert(TK_AND == OP_And, "inconsistent token/opcode definition"); > +static_assert(TK_OR == OP_Or, "inconsistent token/opcode definition"); > +static_assert(TK_PLUS == OP_Add, "inconsistent token/opcode definition"); > +static_assert(TK_MINUS == OP_Subtract, "inconsistent token/opcode definition"); > +static_assert(TK_REM == OP_Remainder, "inconsistent token/opcode definition"); > +static_assert(TK_BITAND == OP_BitAnd, "inconsistent token/opcode definition"); > +static_assert(TK_BITOR == OP_BitOr, "inconsistent token/opcode definition"); > +static_assert(TK_SLASH == OP_Divide, "inconsistent token/opcode definition"); > +static_assert(TK_LSHIFT == OP_ShiftLeft, "inconsistent token/opcode definition"); > +static_assert(TK_RSHIFT == OP_ShiftRight, "inconsistent token/opcode definition"); > +static_assert(TK_CONCAT == OP_Concat, "inconsistent token/opcode definition"); > +static_assert(TK_BITNOT == OP_BitNot, "inconsistent token/opcode definition"); > +static_assert(TK_NOT == OP_Not, "inconsistent token/opcode definition"); > +static_assert(TK_ISNULL == OP_IsNull, "inconsistent token/opcode definition"); > +static_assert(TK_NOTNULL == OP_NotNull, "inconsistent token/opcode definition"); > + > /* > * Generate code into the current Vdbe to evaluate the given > * expression. Attempt to store the results in register "target". > @@ -3839,28 +3864,16 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > ®Free2); > codeCompare(pParse, pLeft, pExpr->pRight, op, > r1, r2, inReg, SQL_STOREP2); > - static_assert(TK_LT == OP_Lt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Lt); > VdbeCoverageIf(v, op == OP_Lt); > - static_assert(TK_LE == OP_Le, > - "inconsistent token/opcode definition"); > testcase(op == OP_Le); > VdbeCoverageIf(v, op == OP_Le); > - static_assert(TK_GT == OP_Gt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Gt); > VdbeCoverageIf(v, op == OP_Gt); > - static_assert(TK_GE == OP_Ge, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ge); > VdbeCoverageIf(v, op == OP_Ge); > - static_assert(TK_EQ == OP_Eq, > - "inconsistent token/opcode definition"); > testcase(op == OP_Eq); > VdbeCoverageIf(v, op == OP_Eq); > - static_assert(TK_NE == OP_Ne, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ne); > VdbeCoverageIf(v, op == OP_Ne); > testcase(regFree1 == 0); > @@ -3880,38 +3893,16 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > case TK_LSHIFT: > case TK_RSHIFT: > case TK_CONCAT:{ > - static_assert(TK_AND == OP_And, > - "inconsistent token/opcode definition"); > testcase(op == TK_AND); > - static_assert(TK_OR == OP_Or, > - "inconsistent token/opcode definition"); > testcase(op == TK_OR); > - static_assert(TK_PLUS == OP_Add, > - "inconsistent token/opcode definition"); > testcase(op == TK_PLUS); > - static_assert(TK_MINUS == OP_Subtract, > - "inconsistent token/opcode definition"); > testcase(op == TK_MINUS); > - static_assert(TK_REM == OP_Remainder, > - "inconsistent token/opcode definition"); > testcase(op == TK_REM); > - static_assert(TK_BITAND == OP_BitAnd, > - "inconsistent token/opcode definition"); > testcase(op == TK_BITAND); > - static_assert(TK_BITOR == OP_BitOr, > - "inconsistent token/opcode definition"); > testcase(op == TK_BITOR); > - static_assert(TK_SLASH == OP_Divide, > - "inconsistent token/opcode definition"); > testcase(op == TK_SLASH); > - static_assert(TK_LSHIFT == OP_ShiftLeft, > - "inconsistent token/opcode definition"); > testcase(op == TK_LSHIFT); > - static_assert(TK_RSHIFT == OP_ShiftRight, > - "inconsistent token/opcode definition"); > testcase(op == TK_RSHIFT); > - static_assert(TK_CONCAT == OP_Concat, > - "inconsistent token/opcode definition"); > testcase(op == TK_CONCAT); > r1 = sqlExprCodeTemp(pParse, pExpr->pLeft, > ®Free1); > @@ -3948,11 +3939,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > } > case TK_BITNOT: > case TK_NOT:{ > - static_assert(TK_BITNOT == OP_BitNot, > - "inconsistent token/opcode definition"); > testcase(op == TK_BITNOT); > - static_assert(TK_NOT == OP_Not, > - "inconsistent token/opcode definition"); > testcase(op == TK_NOT); > r1 = sqlExprCodeTemp(pParse, pExpr->pLeft, > ®Free1); > @@ -3963,11 +3950,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > case TK_ISNULL: > case TK_NOTNULL:{ > int addr; > - static_assert(TK_ISNULL == OP_IsNull, > - "inconsistent token/opcode definition"); > testcase(op == TK_ISNULL); > - static_assert(TK_NOTNULL == OP_NotNull, > - "inconsistent token/opcode definition"); > testcase(op == TK_NOTNULL); > sqlVdbeAddOp2(v, OP_Bool, true, target); > r1 = sqlExprCodeTemp(pParse, pExpr->pLeft, > @@ -4780,31 +4763,19 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull) > ®Free2); > codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1, > r2, dest, jumpIfNull); > - static_assert(TK_LT == OP_Lt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Lt); > VdbeCoverageIf(v, op == OP_Lt); > - static_assert(TK_LE == OP_Le, > - "inconsistent token/opcode definition"); > testcase(op == OP_Le); > VdbeCoverageIf(v, op == OP_Le); > - static_assert(TK_GT == OP_Gt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Gt); > VdbeCoverageIf(v, op == OP_Gt); > - static_assert(TK_GE == OP_Ge, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ge); > VdbeCoverageIf(v, op == OP_Ge); > - static_assert(TK_EQ == OP_Eq, > - "inconsistent token/opcode definition"); > testcase(op == OP_Eq); > VdbeCoverageIf(v, op == OP_Eq > && jumpIfNull == SQL_NULLEQ); > VdbeCoverageIf(v, op == OP_Eq > && jumpIfNull != SQL_NULLEQ); > - static_assert(TK_NE == OP_Ne, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ne); > VdbeCoverageIf(v, op == OP_Ne > && jumpIfNull == SQL_NULLEQ); > @@ -4816,11 +4787,7 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull) > } > case TK_ISNULL: > case TK_NOTNULL:{ > - static_assert(TK_ISNULL == OP_IsNull, > - "inconsistent token/opcode definition"); > testcase(op == TK_ISNULL); > - static_assert(TK_NOTNULL == OP_NotNull, > - "inconsistent token/opcode definition"); > testcase(op == TK_NOTNULL); > r1 = sqlExprCodeTemp(pParse, pExpr->pLeft, > ®Free1); > @@ -4981,31 +4948,19 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull) > ®Free2); > codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1, > r2, dest, jumpIfNull); > - static_assert(TK_LT == OP_Lt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Lt); > VdbeCoverageIf(v, op == OP_Lt); > - static_assert(TK_LE == OP_Le, > - "inconsistent token/opcode definition"); > testcase(op == OP_Le); > VdbeCoverageIf(v, op == OP_Le); > - static_assert(TK_GT == OP_Gt, > - "inconsistent token/opcode definition"); > testcase(op == OP_Gt); > VdbeCoverageIf(v, op == OP_Gt); > - static_assert(TK_GE == OP_Ge, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ge); > VdbeCoverageIf(v, op == OP_Ge); > - static_assert(TK_EQ == OP_Eq, > - "inconsistent token/opcode definition"); > testcase(op == OP_Eq); > VdbeCoverageIf(v, op == OP_Eq > && jumpIfNull != SQL_NULLEQ); > VdbeCoverageIf(v, op == OP_Eq > && jumpIfNull == SQL_NULLEQ); > - static_assert(TK_NE == OP_Ne, > - "inconsistent token/opcode definition"); > testcase(op == OP_Ne); > VdbeCoverageIf(v, op == OP_Ne > && jumpIfNull != SQL_NULLEQ); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 8560e51a1..414b89272 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -743,6 +743,20 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno, > return 0; > } > > +/* > + * static asserts for sqlVdbeExec > + */ > +static_assert(OP_ShiftRight==OP_ShiftLeft+1, "inconsistent opcode definition"); > +static_assert(OP_SeekLE == OP_SeekLT+1, "inconsistent opcode definition"); > +static_assert(OP_SeekGE == OP_SeekLT+2, "inconsistent opcode definition"); > +static_assert(OP_SeekGT == OP_SeekGE+1, "inconsistent opcode definition"); > +static_assert((OP_SeekLE & 0x0001)==(OP_SeekGT & 0x0001), > + "inconsistent opcode definition"); > +static_assert((OP_SeekLT & 0x0001)==(OP_SeekGE & 0x0001), > + "inconsistent opcode definition"); > +static_assert((OP_IdxLE&1)==(OP_IdxLT&1) && (OP_IdxGE&1)==(OP_IdxGT&1), > + "inconsistent opcode definition"); > + > /* > * Execute as much of a VDBE program as we can. > * This is the core of sql_step(). > @@ -1847,8 +1861,6 @@ case OP_ShiftRight: { /* same as TK_RSHIFT, in1, in2, out3 */ > > /* If shifting by a negative amount, shift in the other direction */ > if (iB<0) { > - static_assert(OP_ShiftRight==OP_ShiftLeft+1, > - "inconsistent opcode definition"); > op = 2*OP_ShiftLeft + 1 - op; > iB = iB>(-64) ? -iB : 64; > } > @@ -3281,12 +3293,6 @@ case OP_SeekGT: { /* jump, in3 */ > pC = p->apCsr[pOp->p1]; > assert(pC!=0); > assert(pC->eCurType==CURTYPE_TARANTOOL); > - static_assert(OP_SeekLE == OP_SeekLT+1, > - "inconsistent opcode definition"); > - static_assert(OP_SeekGE == OP_SeekLT+2, > - "inconsistent opcode definition"); > - static_assert(OP_SeekGT == OP_SeekLT+3, > - "inconsistent opcode definition"); > assert(pC->uc.pCursor!=0); > oc = pOp->opcode; > eqOnly = 0; > @@ -3355,26 +3361,14 @@ case OP_SeekGT: { /* jump, in3 */ > * (x > 4.9) -> (x >= 5) > * (x <= 4.9) -> (x < 5) > */ > - if (pIn3->u.r<(double)iKey) { > - static_assert(OP_SeekGE==(OP_SeekGT-1), > - "inconsistent opcode definition"); > - static_assert(OP_SeekLT==(OP_SeekLE-1), > - "inconsistent opcode definition"); > - static_assert((OP_SeekLE & 0x0001)==(OP_SeekGT & 0x0001), > - "inconsistent opcode definition"); > + if (pIn3->u.r < (double)iKey) { > if ((oc & 0x0001)==(OP_SeekGT & 0x0001)) oc--; > } > > /* If the approximation iKey is smaller than the actual real search > * term, substitute <= for < and > for >=. > */ > - else if (pIn3->u.r>(double)iKey) { > - static_assert(OP_SeekLE==(OP_SeekLT+1), > - "inconsistent opcode definition"); > - static_assert(OP_SeekGT==(OP_SeekGE+1), > - "inconsistent opcode definition"); > - static_assert((OP_SeekLT & 0x0001)==(OP_SeekGE & 0x0001), > - "inconsistent opcode definition"); > + else if (pIn3->u.r > (double)iKey) { > if ((oc & 0x0001)==(OP_SeekLT & 0x0001)) oc++; > } > } > @@ -4539,8 +4533,6 @@ case OP_IdxGE: { /* jump */ > { int i; for(i=0; i #endif > int res = tarantoolsqlIdxKeyCompare(pC->uc.pCursor, &r); > - static_assert((OP_IdxLE&1)==(OP_IdxLT&1) && (OP_IdxGE&1)==(OP_IdxGT&1), > - "inconsistent opcode definition"); > if ((pOp->opcode&1)==(OP_IdxLT&1)) { > assert(pOp->opcode==OP_IdxLE || pOp->opcode==OP_IdxLT); > res = -res; > diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c > index 2d29c0bc7..3b7d51f80 100644 > --- a/src/box/sql/whereexpr.c > +++ b/src/box/sql/whereexpr.c > @@ -124,6 +124,15 @@ whereClauseInsert(WhereClause * pWC, Expr * p, u16 wtFlags) > return idx; > } > > +/* > + * static asserts for allowedOp, exprCommute, exprMightBeIndexed > + */ > +static_assert(TK_GE == TK_EQ + 4, "inconsistent token definition"); > +static_assert(TK_LT == TK_GT + 2, "inconsistent token definition"); > +static_assert(TK_LE == TK_GT + 1, "inconsistent token definition"); > +static_assert(TK_GE == TK_LE + 2, "inconsistent token definition"); > +static_assert(TK_IN < TK_GE, "inconsistent token definition"); > + > /* > * Return TRUE if the given operator is one of the operators that is > * allowed for an indexable WHERE clause term. The allowed operators are > @@ -132,14 +141,6 @@ whereClauseInsert(WhereClause * pWC, Expr * p, u16 wtFlags) > static int > allowedOp(int op) > { > - static_assert(TK_GT > TK_EQ && TK_GT < TK_GE, > - "inconsistent token definition"); > - static_assert(TK_LT > TK_EQ && TK_LT < TK_GE, > - "inconsistent token definition"); > - static_assert(TK_LE > TK_EQ && TK_LE < TK_GE, > - "inconsistent token definition"); > - static_assert(TK_GE == TK_EQ + 4, > - "inconsistent token definition"); > return op == TK_IN || (op >= TK_EQ && op <= TK_GE) || op == TK_ISNULL; > } > > @@ -190,14 +191,6 @@ exprCommute(Parse * pParse, Expr * pExpr) > } > SWAP(pExpr->pRight, pExpr->pLeft); > if (pExpr->op >= TK_GT) { > - static_assert(TK_LT == TK_GT + 2, > - "inconsistent token definition"); > - static_assert(TK_GE == TK_LE + 2, > - "inconsistent token definition"); > - static_assert(TK_GT > TK_EQ, > - "inconsistent token definition"); > - static_assert(TK_GT < TK_LE, > - "inconsistent token definition"); > assert(pExpr->op >= TK_GT && pExpr->op <= TK_GE); > pExpr->op = ((pExpr->op - TK_GT) ^ 2) + TK_GT; > } > @@ -953,9 +946,6 @@ exprMightBeIndexed(int op, /* The specific comparison operator */ > * inequality constraint (>, <, >= or <=), perform the processing > * on the first element of the vector. > */ > - static_assert(TK_GT + 1 == TK_LE && TK_GT + 2 == TK_LT && TK_GT + 3 == TK_GE, > - "inconsistent token definition"); > - static_assert(TK_IN < TK_GE, "inconsistent token definition"); > assert(op <= TK_GE || op == TK_ISNULL || op == TK_NOTNULL); > if (pExpr->op == TK_VECTOR && (op >= TK_GT && op <= TK_GE)) { > pExpr = pExpr->x.pList->a[0].pExpr; > -- > 2.21.0 > >