Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Alexey Zavodchenkov <zavodchen@gmail.com>
Subject: [tarantool-patches] Re: [PATCH] sql: Change token/opcode relation assertions to static
Date: Tue, 27 Aug 2019 19:40:50 +0300	[thread overview]
Message-ID: <A2C8E320-3DD6-4061-937A-A86776F527C4@tarantool.org> (raw)
In-Reply-To: <20190823154958.20837-1-zavodchen@gmail.com>


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

  reply	other threads:[~2019-08-27 16:40 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-23 15:49 [tarantool-patches] " Alexey Zavodchenkov
2019-08-27 16:40 ` n.pettik [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=A2C8E320-3DD6-4061-937A-A86776F527C4@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=zavodchen@gmail.com \
    --subject='[tarantool-patches] Re: [PATCH] sql: Change token/opcode relation assertions to static' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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