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 37456241CF for ; Tue, 5 Mar 2019 04:06:42 -0500 (EST) 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 ykaXE8oTZgMx for ; Tue, 5 Mar 2019 04:06:42 -0500 (EST) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 D5FB120210 for ; Tue, 5 Mar 2019 04:06:41 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v3 4/9] sql: remove field rc of struct Parse From: "n.pettik" In-Reply-To: <50846f935fdc69a4ff17958b8307bc9dc245fe70.1551530224.git.imeevma@gmail.com> Date: Tue, 5 Mar 2019 12:06:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <50846f935fdc69a4ff17958b8307bc9dc245fe70.1551530224.git.imeevma@gmail.com> 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: Imeev Mergen Nit: remove from. > Currently, the field rc of the struct Parse can have only two > values: SQL_OK or SQL_TARANTOOL_ERROR. Therefore, it is logical to > replace it with a new boolean field. This patche replaces field rc > by new field is_aborted. Fixed commit message (original one contained several mistakes): sql: replace rc with is_abort status in stuct Parse =20 Currently, field representing return code in struct Parse can take = only two values: SQL_OK (successfully finished parsing) and SQL_TARANTOOL_ERROR (in case of any errors occurred). Therefore, it = can be replaced with a boolean field. Patch provides straightforward refactoring. =20 Part of #3965 > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 0c6296d..42737ff 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -100,15 +100,15 @@ sqlPrepare(sql * db, /* Database handle. */ > } > assert(0 =3D=3D sParse.nQueryLoop); >=20 > - if (sParse.rc =3D=3D SQL_DONE) > - sParse.rc =3D SQL_OK; > if (db->mallocFailed) { > - sParse.rc =3D SQL_NOMEM; > + diag_set(OutOfMemory, 0, "SQL", "db"); > + sParse.is_aborted =3D true; See comments for previous patches. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 719b85e..42ff4b8 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -2643,7 +2643,6 @@ struct Parse { > sql *db; /* The main database structure */ > char *zErrMsg; /* An error message */ > Vdbe *pVdbe; /* An engine for executing database = bytecode */ > - int rc; /* Return code from execution */ > u8 colNamesSet; /* TRUE after OP_ColumnName has been = issued to pVdbe */ > u8 nTempReg; /* Number of temporary registers in = aTempReg[] */ > u8 isMultiWrite; /* True if statement may modify/insert = multiple rows */ > @@ -2677,6 +2676,8 @@ struct Parse { > u8 eOrconf; /* Default ON CONFLICT policy for = trigger steps */ > /** Region to make SQL temp allocations. */ > struct region region; > + /** Flag to show that parsing should be aborted. */ Comment is misleading: now we don=E2=80=99t abort parsing process, but instead allow it to be finished and raise an error at the end. Fix comment pls. > + bool is_aborted; >=20 > = /*************************************************************************= * > * Fields above must be initialized to zero. The fields that follow, > @@ -534,25 +533,17 @@ sqlRunParser(Parse * pParse, const char *zSql, = char **pzErrMsg) > sqlParserFree(pEngine, sql_free); > if (db->mallocFailed) { > diag_set(OutOfMemory, 0, "SQL", "db"); > - pParse->rc =3D SQL_TARANTOOL_ERROR; > - } > - if (pParse->rc !=3D SQL_OK && pParse->rc !=3D SQL_DONE > - && pParse->zErrMsg =3D=3D 0) { > - const char *error; > - if (is_tarantool_error(pParse->rc) && > - tarantoolErrorMessage() !=3D NULL) > - error =3D tarantoolErrorMessage(); > - else > - error =3D sqlErrStr(pParse->rc); > - pParse->zErrMsg =3D sqlMPrintf(db, "%s", error); > + pParse->is_aborted =3D true; > } > + if (pParse->is_aborted && pParse->zErrMsg =3D=3D 0) > + pParse->zErrMsg =3D sqlMPrintf(db, "%s", = tarantoolErrorMessage()); > assert(pzErrMsg !=3D 0); > if (pParse->zErrMsg) { > *pzErrMsg =3D pParse->zErrMsg; > - sql_log(pParse->rc, "%s", *pzErrMsg); > + sql_log(SQL_TARANTOOL_ERROR, "%s", *pzErrMsg); Do we need this call at all? > diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > index bab6493..0d8bf15 100755 > --- a/test/sql-tap/check.test.lua > +++ b/test/sql-tap/check.test.lua > @@ -516,7 +516,7 @@ test:do_catchsql_test( > ); > ]], { > -- > - 1, "Wrong space options (field 5): invalid expression = specified (SQL error: bindings are not allowed in DDL)" > + 1, "Wrong space options (field 5): invalid expression = specified (bindings are not allowed in DDL)" > -- > }) Why test results have changed if you provided non-functional refactoring?