[tarantool-patches] Re: [PATCH] sql: Change token/opcode relation assertions to static

[n.pettik]

> 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.