Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH] sql: Change token/opcode relation assertions to static
@ 2019-08-23 15:49 Alexey Zavodchenkov
  2019-08-27 16:40 ` [tarantool-patches] " n.pettik
  0 siblings, 1 reply; 6+ messages in thread
From: Alexey Zavodchenkov @ 2019-08-23 15:49 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexey Zavodchenkov

There are several token/opcode relative positions assertions in the sql codebase
and those tokens/opcodes are defined before the source compilation.
So it is possible to change those assertions to static ones.

Closes: #4240

Issue: https://github.com/tarantool/tarantool/issues/4240
Commit in the fork: https://github.com/Separador/tarantool/commit/d36d7c8080
---
 src/box/sql/expr.c      | 105 ++++++++++++++++++++++++++--------------
 src/box/sql/vdbe.c      |  33 ++++++++-----
 src/box/sql/whereexpr.c |  29 +++++++----
 3 files changed, 111 insertions(+), 56 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1f9d91705..668562896 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3839,22 +3839,28 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 							 &regFree2);
 				codeCompare(pParse, pLeft, pExpr->pRight, op,
 					    r1, r2, inReg, SQL_STOREP2);
-				assert(TK_LT == OP_Lt);
+				static_assert(TK_LT == OP_Lt,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Lt);
 				VdbeCoverageIf(v, op == OP_Lt);
-				assert(TK_LE == OP_Le);
+				static_assert(TK_LE == OP_Le,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Le);
 				VdbeCoverageIf(v, op == OP_Le);
-				assert(TK_GT == OP_Gt);
+				static_assert(TK_GT == OP_Gt,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Gt);
 				VdbeCoverageIf(v, op == OP_Gt);
-				assert(TK_GE == OP_Ge);
+				static_assert(TK_GE == OP_Ge,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Ge);
 				VdbeCoverageIf(v, op == OP_Ge);
-				assert(TK_EQ == OP_Eq);
+				static_assert(TK_EQ == OP_Eq,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Eq);
 				VdbeCoverageIf(v, op == OP_Eq);
-				assert(TK_NE == OP_Ne);
+				static_assert(TK_NE == OP_Ne,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Ne);
 				VdbeCoverageIf(v, op == OP_Ne);
 				testcase(regFree1 == 0);
@@ -3874,27 +3880,38 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_LSHIFT:
 	case TK_RSHIFT:
 	case TK_CONCAT:{
-			assert(TK_AND == OP_And);
+			static_assert(TK_AND == OP_And,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_AND);
-			assert(TK_OR == OP_Or);
+			static_assert(TK_OR == OP_Or,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_OR);
-			assert(TK_PLUS == OP_Add);
+			static_assert(TK_PLUS == OP_Add,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_PLUS);
-			assert(TK_MINUS == OP_Subtract);
+			static_assert(TK_MINUS == OP_Subtract,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_MINUS);
-			assert(TK_REM == OP_Remainder);
+			static_assert(TK_REM == OP_Remainder,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_REM);
-			assert(TK_BITAND == OP_BitAnd);
+			static_assert(TK_BITAND == OP_BitAnd,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITAND);
-			assert(TK_BITOR == OP_BitOr);
+			static_assert(TK_BITOR == OP_BitOr,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITOR);
-			assert(TK_SLASH == OP_Divide);
+			static_assert(TK_SLASH == OP_Divide,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_SLASH);
-			assert(TK_LSHIFT == OP_ShiftLeft);
+			static_assert(TK_LSHIFT == OP_ShiftLeft,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_LSHIFT);
-			assert(TK_RSHIFT == OP_ShiftRight);
+			static_assert(TK_RSHIFT == OP_ShiftRight,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_RSHIFT);
-			assert(TK_CONCAT == OP_Concat);
+			static_assert(TK_CONCAT == OP_Concat,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_CONCAT);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -3931,9 +3948,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		}
 	case TK_BITNOT:
 	case TK_NOT:{
-			assert(TK_BITNOT == OP_BitNot);
+			static_assert(TK_BITNOT == OP_BitNot,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITNOT);
-			assert(TK_NOT == OP_Not);
+			static_assert(TK_NOT == OP_Not,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_NOT);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -3944,9 +3963,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_ISNULL:
 	case TK_NOTNULL:{
 			int addr;
-			assert(TK_ISNULL == OP_IsNull);
+			static_assert(TK_ISNULL == OP_IsNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_ISNULL);
-			assert(TK_NOTNULL == OP_NotNull);
+			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,
@@ -4759,25 +4780,31 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 						 &regFree2);
 			codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1,
 				    r2, dest, jumpIfNull);
-			assert(TK_LT == OP_Lt);
+			static_assert(TK_LT == OP_Lt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Lt);
 			VdbeCoverageIf(v, op == OP_Lt);
-			assert(TK_LE == OP_Le);
+			static_assert(TK_LE == OP_Le,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Le);
 			VdbeCoverageIf(v, op == OP_Le);
-			assert(TK_GT == OP_Gt);
+			static_assert(TK_GT == OP_Gt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Gt);
 			VdbeCoverageIf(v, op == OP_Gt);
-			assert(TK_GE == OP_Ge);
+			static_assert(TK_GE == OP_Ge,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ge);
 			VdbeCoverageIf(v, op == OP_Ge);
-			assert(TK_EQ == OP_Eq);
+			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);
-			assert(TK_NE == OP_Ne);
+			static_assert(TK_NE == OP_Ne,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ne);
 			VdbeCoverageIf(v, op == OP_Ne
 				       && jumpIfNull == SQL_NULLEQ);
@@ -4789,9 +4816,11 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 		}
 	case TK_ISNULL:
 	case TK_NOTNULL:{
-			assert(TK_ISNULL == OP_IsNull);
+			static_assert(TK_ISNULL == OP_IsNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_ISNULL);
-			assert(TK_NOTNULL == OP_NotNull);
+			static_assert(TK_NOTNULL == OP_NotNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_NOTNULL);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -4952,25 +4981,31 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 						 &regFree2);
 			codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1,
 				    r2, dest, jumpIfNull);
-			assert(TK_LT == OP_Lt);
+			static_assert(TK_LT == OP_Lt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Lt);
 			VdbeCoverageIf(v, op == OP_Lt);
-			assert(TK_LE == OP_Le);
+			static_assert(TK_LE == OP_Le,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Le);
 			VdbeCoverageIf(v, op == OP_Le);
-			assert(TK_GT == OP_Gt);
+			static_assert(TK_GT == OP_Gt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Gt);
 			VdbeCoverageIf(v, op == OP_Gt);
-			assert(TK_GE == OP_Ge);
+			static_assert(TK_GE == OP_Ge,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ge);
 			VdbeCoverageIf(v, op == OP_Ge);
-			assert(TK_EQ == OP_Eq);
+			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);
-			assert(TK_NE == OP_Ne);
+			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 dced8fc73..8560e51a1 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1847,7 +1847,8 @@ case OP_ShiftRight: {           /* same as TK_RSHIFT, in1, in2, out3 */
 
 		/* If shifting by a negative amount, shift in the other direction */
 		if (iB<0) {
-			assert(OP_ShiftRight==OP_ShiftLeft+1);
+			static_assert(OP_ShiftRight==OP_ShiftLeft+1,
+				"inconsistent opcode definition");
 			op = 2*OP_ShiftLeft + 1 - op;
 			iB = iB>(-64) ? -iB : 64;
 		}
@@ -3280,9 +3281,12 @@ case OP_SeekGT: {       /* jump, in3 */
 	pC = p->apCsr[pOp->p1];
 	assert(pC!=0);
 	assert(pC->eCurType==CURTYPE_TARANTOOL);
-	assert(OP_SeekLE == OP_SeekLT+1);
-	assert(OP_SeekGE == OP_SeekLT+2);
-	assert(OP_SeekGT == OP_SeekLT+3);
+	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;
@@ -3352,9 +3356,12 @@ case OP_SeekGT: {       /* jump, in3 */
 			 *        (x <= 4.9)    ->     (x <  5)
 			 */
 			if (pIn3->u.r<(double)iKey) {
-				assert(OP_SeekGE==(OP_SeekGT-1));
-				assert(OP_SeekLT==(OP_SeekLE-1));
-				assert((OP_SeekLE & 0x0001)==(OP_SeekGT & 0x0001));
+				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 ((oc & 0x0001)==(OP_SeekGT & 0x0001)) oc--;
 			}
 
@@ -3362,9 +3369,12 @@ case OP_SeekGT: {       /* jump, in3 */
 			 * term, substitute <= for < and > for >=.
 			 */
 			else if (pIn3->u.r>(double)iKey) {
-				assert(OP_SeekLE==(OP_SeekLT+1));
-				assert(OP_SeekGT==(OP_SeekGE+1));
-				assert((OP_SeekLT & 0x0001)==(OP_SeekGE & 0x0001));
+				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");
 				if ((oc & 0x0001)==(OP_SeekLT & 0x0001)) oc++;
 			}
 		}
@@ -4529,7 +4539,8 @@ 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);
-	assert((OP_IdxLE&1)==(OP_IdxLT&1) && (OP_IdxGE&1)==(OP_IdxGT&1));
+	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 8adf6a5f1..2d29c0bc7 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -132,10 +132,14 @@ whereClauseInsert(WhereClause * pWC, Expr * p, u16 wtFlags)
 static int
 allowedOp(int op)
 {
-	assert(TK_GT > TK_EQ && TK_GT < TK_GE);
-	assert(TK_LT > TK_EQ && TK_LT < TK_GE);
-	assert(TK_LE > TK_EQ && TK_LE < TK_GE);
-	assert(TK_GE == TK_EQ + 4);
+	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;
 }
 
@@ -186,10 +190,14 @@ exprCommute(Parse * pParse, Expr * pExpr)
 	}
 	SWAP(pExpr->pRight, pExpr->pLeft);
 	if (pExpr->op >= TK_GT) {
-		assert(TK_LT == TK_GT + 2);
-		assert(TK_GE == TK_LE + 2);
-		assert(TK_GT > TK_EQ);
-		assert(TK_GT < TK_LE);
+		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;
 	}
@@ -945,8 +953,9 @@ exprMightBeIndexed(int op,	/* The specific comparison operator */
 	 * inequality constraint (>, <, >= or <=), perform the processing
 	 * on the first element of the vector.
 	 */
-	assert(TK_GT + 1 == TK_LE && TK_GT + 2 == TK_LT && TK_GT + 3 == TK_GE);
-	assert(TK_IN < TK_GE);
+	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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: Change token/opcode relation assertions to static
  2019-08-23 15:49 [tarantool-patches] [PATCH] sql: Change token/opcode relation assertions to static Alexey Zavodchenkov
@ 2019-08-27 16:40 ` n.pettik
  2019-08-30 15:47   ` [tarantool-patches] [PATCH 0/2] " Alexey Zavodchenkov
  0 siblings, 1 reply; 6+ messages in thread
From: n.pettik @ 2019-08-27 16:40 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexey Zavodchenkov


> There are several token/opcode relative positions assertions in the sql codebase
> and those tokens/opcodes are defined before the source compilation.
> So it is possible to change those assertions to static ones.
> 
> Closes: #4240
> 
> Issue: https://github.com/tarantool/tarantool/issues/4240
> Commit in the fork: https://github.com/Separador/tarantool/commit/d36d7c8080

Hello,

Nit: please, put links to issue and branch below delimiter ---

Unfortunately, I failed to apply your patch neither using git am nor
git apply. Make sure that you are using git format-patch utility and
don’t manually change patch context.

> ---
> src/box/sql/expr.c      | 105 ++++++++++++++++++++++++++--------------
> src/box/sql/vdbe.c      |  33 ++++++++-----
> src/box/sql/whereexpr.c |  29 +++++++----
> 3 files changed, 111 insertions(+), 56 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 1f9d91705..668562896 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3839,22 +3839,28 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> 							 &regFree2);
> 				codeCompare(pParse, pLeft, pExpr->pRight, op,
> 					    r1, r2, inReg, SQL_STOREP2);
> -				assert(TK_LT == OP_Lt);
> +				static_assert(TK_LT == OP_Lt,
> +					"inconsistent token/opcode definition");
> 				testcase(op == OP_Lt);
> 				VdbeCoverageIf(v, op == OP_Lt);
> -				assert(TK_LE == OP_Le);
> +				static_assert(TK_LE == OP_Le,
> +					"inconsistent token/opcode definition”);

Nit: I’d group and move out of function all these assertion to the one place.
For instance, asserts in sqlExprCodeTarget() and sqlExprIfTrue() verify
that token TK_XX corresponds to OP_Xx opcode. Hence let's move them,
before definition of sqlExprCodeTarget().

The same applies for group of assertions in VDBE verifying relative opcode  
positions and in query planner (whereexpr.c) checking relative token positions.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tarantool-patches] [PATCH 0/2] Re: [PATCH] sql: Change token/opcode relation assertions to static
  2019-08-27 16:40 ` [tarantool-patches] " n.pettik
@ 2019-08-30 15:47   ` Alexey Zavodchenkov
  2019-08-30 15:48     ` [tarantool-patches] [PATCH 1/2] sql: Change token/opcode relative positions assertion " Alexey Zavodchenkov
  2019-08-30 15:48     ` [tarantool-patches] [PATCH 2/2] Group assertions and move them out of functions Alexey Zavodchenkov
  0 siblings, 2 replies; 6+ messages in thread
From: Alexey Zavodchenkov @ 2019-08-30 15:47 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexey Zavodchenkov

Thank you for the review!
I'm resending the intial patch
and changes you suggested as the separate patch.

Branch: https://github.com/Separador/tarantool/tree/gh-4240 

Alexey Zavodchenkov (2):
  sql: Change token/opcode relative positions assertion to static
  Group assertions and move them out of functions

 src/box/sql/expr.c      | 60 +++++++++++++++++------------------------
 src/box/sql/vdbe.c      | 29 +++++++++++---------
 src/box/sql/whereexpr.c | 19 +++++++------
 3 files changed, 50 insertions(+), 58 deletions(-)

-- 
2.21.0

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tarantool-patches] [PATCH 1/2] sql: Change token/opcode relative positions assertion to static
  2019-08-30 15:47   ` [tarantool-patches] [PATCH 0/2] " Alexey Zavodchenkov
@ 2019-08-30 15:48     ` Alexey Zavodchenkov
  2019-08-30 15:48     ` [tarantool-patches] [PATCH 2/2] Group assertions and move them out of functions Alexey Zavodchenkov
  1 sibling, 0 replies; 6+ messages in thread
From: Alexey Zavodchenkov @ 2019-08-30 15:48 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexey Zavodchenkov

There are several token/opcode relative positions assertions in the sql codebase
and those tokens/opcodes are defined before the source compilation.
So it is possible to change those assertions to static ones.

Closes: #4240
---
 src/box/sql/expr.c      | 105 ++++++++++++++++++++++++++--------------
 src/box/sql/vdbe.c      |  33 ++++++++-----
 src/box/sql/whereexpr.c |  29 +++++++----
 3 files changed, 111 insertions(+), 56 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1f9d91705..668562896 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3839,22 +3839,28 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 							 &regFree2);
 				codeCompare(pParse, pLeft, pExpr->pRight, op,
 					    r1, r2, inReg, SQL_STOREP2);
-				assert(TK_LT == OP_Lt);
+				static_assert(TK_LT == OP_Lt,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Lt);
 				VdbeCoverageIf(v, op == OP_Lt);
-				assert(TK_LE == OP_Le);
+				static_assert(TK_LE == OP_Le,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Le);
 				VdbeCoverageIf(v, op == OP_Le);
-				assert(TK_GT == OP_Gt);
+				static_assert(TK_GT == OP_Gt,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Gt);
 				VdbeCoverageIf(v, op == OP_Gt);
-				assert(TK_GE == OP_Ge);
+				static_assert(TK_GE == OP_Ge,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Ge);
 				VdbeCoverageIf(v, op == OP_Ge);
-				assert(TK_EQ == OP_Eq);
+				static_assert(TK_EQ == OP_Eq,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Eq);
 				VdbeCoverageIf(v, op == OP_Eq);
-				assert(TK_NE == OP_Ne);
+				static_assert(TK_NE == OP_Ne,
+					"inconsistent token/opcode definition");
 				testcase(op == OP_Ne);
 				VdbeCoverageIf(v, op == OP_Ne);
 				testcase(regFree1 == 0);
@@ -3874,27 +3880,38 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_LSHIFT:
 	case TK_RSHIFT:
 	case TK_CONCAT:{
-			assert(TK_AND == OP_And);
+			static_assert(TK_AND == OP_And,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_AND);
-			assert(TK_OR == OP_Or);
+			static_assert(TK_OR == OP_Or,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_OR);
-			assert(TK_PLUS == OP_Add);
+			static_assert(TK_PLUS == OP_Add,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_PLUS);
-			assert(TK_MINUS == OP_Subtract);
+			static_assert(TK_MINUS == OP_Subtract,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_MINUS);
-			assert(TK_REM == OP_Remainder);
+			static_assert(TK_REM == OP_Remainder,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_REM);
-			assert(TK_BITAND == OP_BitAnd);
+			static_assert(TK_BITAND == OP_BitAnd,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITAND);
-			assert(TK_BITOR == OP_BitOr);
+			static_assert(TK_BITOR == OP_BitOr,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITOR);
-			assert(TK_SLASH == OP_Divide);
+			static_assert(TK_SLASH == OP_Divide,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_SLASH);
-			assert(TK_LSHIFT == OP_ShiftLeft);
+			static_assert(TK_LSHIFT == OP_ShiftLeft,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_LSHIFT);
-			assert(TK_RSHIFT == OP_ShiftRight);
+			static_assert(TK_RSHIFT == OP_ShiftRight,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_RSHIFT);
-			assert(TK_CONCAT == OP_Concat);
+			static_assert(TK_CONCAT == OP_Concat,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_CONCAT);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -3931,9 +3948,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		}
 	case TK_BITNOT:
 	case TK_NOT:{
-			assert(TK_BITNOT == OP_BitNot);
+			static_assert(TK_BITNOT == OP_BitNot,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_BITNOT);
-			assert(TK_NOT == OP_Not);
+			static_assert(TK_NOT == OP_Not,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_NOT);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -3944,9 +3963,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_ISNULL:
 	case TK_NOTNULL:{
 			int addr;
-			assert(TK_ISNULL == OP_IsNull);
+			static_assert(TK_ISNULL == OP_IsNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_ISNULL);
-			assert(TK_NOTNULL == OP_NotNull);
+			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,
@@ -4759,25 +4780,31 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 						 &regFree2);
 			codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1,
 				    r2, dest, jumpIfNull);
-			assert(TK_LT == OP_Lt);
+			static_assert(TK_LT == OP_Lt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Lt);
 			VdbeCoverageIf(v, op == OP_Lt);
-			assert(TK_LE == OP_Le);
+			static_assert(TK_LE == OP_Le,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Le);
 			VdbeCoverageIf(v, op == OP_Le);
-			assert(TK_GT == OP_Gt);
+			static_assert(TK_GT == OP_Gt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Gt);
 			VdbeCoverageIf(v, op == OP_Gt);
-			assert(TK_GE == OP_Ge);
+			static_assert(TK_GE == OP_Ge,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ge);
 			VdbeCoverageIf(v, op == OP_Ge);
-			assert(TK_EQ == OP_Eq);
+			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);
-			assert(TK_NE == OP_Ne);
+			static_assert(TK_NE == OP_Ne,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ne);
 			VdbeCoverageIf(v, op == OP_Ne
 				       && jumpIfNull == SQL_NULLEQ);
@@ -4789,9 +4816,11 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 		}
 	case TK_ISNULL:
 	case TK_NOTNULL:{
-			assert(TK_ISNULL == OP_IsNull);
+			static_assert(TK_ISNULL == OP_IsNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_ISNULL);
-			assert(TK_NOTNULL == OP_NotNull);
+			static_assert(TK_NOTNULL == OP_NotNull,
+				"inconsistent token/opcode definition");
 			testcase(op == TK_NOTNULL);
 			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
 						 &regFree1);
@@ -4952,25 +4981,31 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 						 &regFree2);
 			codeCompare(pParse, pExpr->pLeft, pExpr->pRight, op, r1,
 				    r2, dest, jumpIfNull);
-			assert(TK_LT == OP_Lt);
+			static_assert(TK_LT == OP_Lt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Lt);
 			VdbeCoverageIf(v, op == OP_Lt);
-			assert(TK_LE == OP_Le);
+			static_assert(TK_LE == OP_Le,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Le);
 			VdbeCoverageIf(v, op == OP_Le);
-			assert(TK_GT == OP_Gt);
+			static_assert(TK_GT == OP_Gt,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Gt);
 			VdbeCoverageIf(v, op == OP_Gt);
-			assert(TK_GE == OP_Ge);
+			static_assert(TK_GE == OP_Ge,
+				"inconsistent token/opcode definition");
 			testcase(op == OP_Ge);
 			VdbeCoverageIf(v, op == OP_Ge);
-			assert(TK_EQ == OP_Eq);
+			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);
-			assert(TK_NE == OP_Ne);
+			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 dced8fc73..8560e51a1 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1847,7 +1847,8 @@ case OP_ShiftRight: {           /* same as TK_RSHIFT, in1, in2, out3 */
 
 		/* If shifting by a negative amount, shift in the other direction */
 		if (iB<0) {
-			assert(OP_ShiftRight==OP_ShiftLeft+1);
+			static_assert(OP_ShiftRight==OP_ShiftLeft+1,
+				"inconsistent opcode definition");
 			op = 2*OP_ShiftLeft + 1 - op;
 			iB = iB>(-64) ? -iB : 64;
 		}
@@ -3280,9 +3281,12 @@ case OP_SeekGT: {       /* jump, in3 */
 	pC = p->apCsr[pOp->p1];
 	assert(pC!=0);
 	assert(pC->eCurType==CURTYPE_TARANTOOL);
-	assert(OP_SeekLE == OP_SeekLT+1);
-	assert(OP_SeekGE == OP_SeekLT+2);
-	assert(OP_SeekGT == OP_SeekLT+3);
+	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;
@@ -3352,9 +3356,12 @@ case OP_SeekGT: {       /* jump, in3 */
 			 *        (x <= 4.9)    ->     (x <  5)
 			 */
 			if (pIn3->u.r<(double)iKey) {
-				assert(OP_SeekGE==(OP_SeekGT-1));
-				assert(OP_SeekLT==(OP_SeekLE-1));
-				assert((OP_SeekLE & 0x0001)==(OP_SeekGT & 0x0001));
+				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 ((oc & 0x0001)==(OP_SeekGT & 0x0001)) oc--;
 			}
 
@@ -3362,9 +3369,12 @@ case OP_SeekGT: {       /* jump, in3 */
 			 * term, substitute <= for < and > for >=.
 			 */
 			else if (pIn3->u.r>(double)iKey) {
-				assert(OP_SeekLE==(OP_SeekLT+1));
-				assert(OP_SeekGT==(OP_SeekGE+1));
-				assert((OP_SeekLT & 0x0001)==(OP_SeekGE & 0x0001));
+				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");
 				if ((oc & 0x0001)==(OP_SeekLT & 0x0001)) oc++;
 			}
 		}
@@ -4529,7 +4539,8 @@ 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);
-	assert((OP_IdxLE&1)==(OP_IdxLT&1) && (OP_IdxGE&1)==(OP_IdxGT&1));
+	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 8adf6a5f1..2d29c0bc7 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -132,10 +132,14 @@ whereClauseInsert(WhereClause * pWC, Expr * p, u16 wtFlags)
 static int
 allowedOp(int op)
 {
-	assert(TK_GT > TK_EQ && TK_GT < TK_GE);
-	assert(TK_LT > TK_EQ && TK_LT < TK_GE);
-	assert(TK_LE > TK_EQ && TK_LE < TK_GE);
-	assert(TK_GE == TK_EQ + 4);
+	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;
 }
 
@@ -186,10 +190,14 @@ exprCommute(Parse * pParse, Expr * pExpr)
 	}
 	SWAP(pExpr->pRight, pExpr->pLeft);
 	if (pExpr->op >= TK_GT) {
-		assert(TK_LT == TK_GT + 2);
-		assert(TK_GE == TK_LE + 2);
-		assert(TK_GT > TK_EQ);
-		assert(TK_GT < TK_LE);
+		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;
 	}
@@ -945,8 +953,9 @@ exprMightBeIndexed(int op,	/* The specific comparison operator */
 	 * inequality constraint (>, <, >= or <=), perform the processing
 	 * on the first element of the vector.
 	 */
-	assert(TK_GT + 1 == TK_LE && TK_GT + 2 == TK_LT && TK_GT + 3 == TK_GE);
-	assert(TK_IN < TK_GE);
+	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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tarantool-patches] [PATCH 2/2] Group assertions and move them out of functions
  2019-08-30 15:47   ` [tarantool-patches] [PATCH 0/2] " Alexey Zavodchenkov
  2019-08-30 15:48     ` [tarantool-patches] [PATCH 1/2] sql: Change token/opcode relative positions assertion " Alexey Zavodchenkov
@ 2019-08-30 15:48     ` Alexey Zavodchenkov
  2019-09-10 23:14       ` [tarantool-patches] " Nikita Pettik
  1 sibling, 1 reply; 6+ messages in thread
From: Alexey Zavodchenkov @ 2019-08-30 15:48 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Alexey Zavodchenkov

---
 src/box/sql/expr.c      | 95 +++++++++++------------------------------
 src/box/sql/vdbe.c      | 40 +++++++----------
 src/box/sql/whereexpr.c | 28 ++++--------
 3 files changed, 50 insertions(+), 113 deletions(-)

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
+ */
+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)
 							 &regFree2);
 				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,
 						 &regFree1);
@@ -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,
 						 &regFree1);
@@ -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)
 						 &regFree2);
 			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,
 						 &regFree1);
@@ -4981,31 +4948,19 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 						 &regFree2);
 			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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] Group assertions and move them out of functions
  2019-08-30 15:48     ` [tarantool-patches] [PATCH 2/2] Group assertions and move them out of functions Alexey Zavodchenkov
@ 2019-09-10 23:14       ` Nikita Pettik
  0 siblings, 0 replies; 6+ messages in thread
From: Nikita Pettik @ 2019-09-10 23:14 UTC (permalink / raw)
  To: tarantool-patches; +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)
>  							 &regFree2);
>  				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,
>  						 &regFree1);
> @@ -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,
>  						 &regFree1);
> @@ -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)
>  						 &regFree2);
>  			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,
>  						 &regFree1);
> @@ -4981,31 +4948,19 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
>  						 &regFree2);
>  			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
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-10 23:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 15:49 [tarantool-patches] [PATCH] sql: Change token/opcode relation assertions to static Alexey Zavodchenkov
2019-08-27 16:40 ` [tarantool-patches] " n.pettik
2019-08-30 15:47   ` [tarantool-patches] [PATCH 0/2] " Alexey Zavodchenkov
2019-08-30 15:48     ` [tarantool-patches] [PATCH 1/2] sql: Change token/opcode relative positions assertion " Alexey Zavodchenkov
2019-08-30 15:48     ` [tarantool-patches] [PATCH 2/2] Group assertions and move them out of functions Alexey Zavodchenkov
2019-09-10 23:14       ` [tarantool-patches] " Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox