[tarantool-patches] Re: [PATCH 2/2] Group assertions and move them out of functions
Nikita Pettik
korablev at tarantool.org
Wed Sep 11 02:14:09 MSK 2019
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<r.nField; i++) assert(memIsValid(&r.aMem[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
>
>
More information about the Tarantool-patches
mailing list