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 C301729BB3 for ; Tue, 19 Mar 2019 05:54:49 -0400 (EDT) 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 J8gdE6-tXsfD for ; Tue, 19 Mar 2019 05:54:49 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 4659929BAD for ; Tue, 19 Mar 2019 05:54:49 -0400 (EDT) From: "n.pettik" Message-Id: <61975B3B-0FDC-4117-88F9-4EB3A6EC247B@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_D16210ED-551A-45B4-A7D0-0AFB1EC93DCF" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse Date: Tue, 19 Mar 2019 12:54:45 +0300 In-Reply-To: <20190318152804.GA24675@tarantool.org> References: <20190318152804.GA24675@tarantool.org> 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 --Apple-Mail=_D16210ED-551A-45B4-A7D0-0AFB1EC93DCF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 >>>>> 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)" >>>>> -- >>>>> }) >>>>=20 >>>> Why test results have changed if you provided >>>> non-functional refactoring? >>> It become this way because now the error in diag instead of being >>> only in zErrMsg of struct Parse. >>=20 >> So, then it should be related to the previous patch, I guess. >> Otherwise, still don=E2=80=99t understand. Or fix commit message, >> since now it implies that only refactoring was provided. >> Or, what is better - move functional changes to separate patch. >>=20 > I divided this ptch into two: > "sql: remove argument pzErrMsg from sqlRunParser()" > "sql: replace rc with is_aborted status in struct Parse=E2=80=9D Both patches are OK. >>> index a2937a0..c2e5d6b 100644 >>> --- a/src/box/sql.c >>> +++ b/src/box/sql.c >>> @@ -1363,12 +1363,8 @@ = sql_checks_resolve_space_def_reference(ExprList *expr_list, >>>=20 >>> sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, = expr_list); >>> int rc =3D 0; >>> - if (parser.rc !=3D SQL_OK) { >>> - /* Tarantool error may be already set with diag. */ >>> - if (parser.rc !=3D SQL_TARANTOOL_ERROR) >>> - diag_set(ClientError, ER_SQL, parser.zErrMsg); >>> + if (parser.is_aborted) >>> rc =3D -1; >>> - } >>> sql_parser_destroy(&parser); >>> return rc; >>=20 >> diff --git a/src/box/sql.c b/src/box/sql.c >> index c2e5d6bd1..ea71dd101 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -1362,9 +1362,6 @@ sql_checks_resolve_space_def_reference(ExprList = *expr_list, >> parser.parse_only =3D true; >>=20 >> sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, = expr_list); >> - int rc =3D 0; >> - if (parser.is_aborted) >> - rc =3D -1; >> sql_parser_destroy(&parser); >> - return rc; >> + return parser.is_aborted ? -1 : 0; >> } >>=20 > I am not sure that this should be applied. I think it isn't right > to look at field is_aborted of parser after parser destruction. sql_parser_destroy() doesn=E2=80=99t affect is_abort field. On the other hand, mb you are right, and using object after destructor is called may seem strange. Anyway, replace pls =E2=80=98if' with ternary = operator: 3 lines of code fit into one. --Apple-Mail=_D16210ED-551A-45B4-A7D0-0AFB1EC93DCF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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(
      );
  ]], {
      -- <check-5.1>
-        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)"
      -- </check-5.1>
  })

Why = test results have changed if you provided
non-functional = refactoring?
It become this way because now = the error in diag instead of being
only in zErrMsg of = struct Parse.

So, then it = should be related to the previous patch, I guess.
Otherwise,= still don=E2=80=99t understand. Or fix commit message,
since now it implies that only refactoring was provided.
Or, what is better - move functional changes to separate = patch.

I divided this ptch into = two:
"sql: remove = argument pzErrMsg from sqlRunParser()"
"sql: replace rc with is_aborted status in struct = Parse=E2=80=9D

Both = patches are OK.

index a2937a0..c2e5d6b = 100644
--- a/src/box/sql.c
+++ = b/src/box/sql.c
@@ -1363,12 +1363,8 @@ = sql_checks_resolve_space_def_reference(ExprList *expr_list,

= sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, = expr_list);
int rc =3D 0;
- if = (parser.rc !=3D SQL_OK) {
- /* Tarantool error may be already = set with diag. */
- if (parser.rc !=3D = SQL_TARANTOOL_ERROR)
- diag_set(ClientError, ER_SQL, = parser.zErrMsg);
+ if (parser.is_aborted)
= = rc =3D -1;
- }
= sql_parser_destroy(&parser);
return = rc;

diff --git a/src/box/sql.c = b/src/box/sql.c
index c2e5d6bd1..ea71dd101 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1362,9 +1362,6 @@ = sql_checks_resolve_space_def_reference(ExprList *expr_list,
       parser.parse_only =3D= true;

       sql_resolve_self_refe= rence(&parser, def, NC_IsCheck, NULL, expr_list);
- =       int rc =3D 0;
- =       if (parser.is_aborted)
-= =             &n= bsp; rc =3D -1;
       sql_parser_destroy(&a= mp;parser);
-       return = rc;
+       return = parser.is_aborted ? -1 : 0;
}

I am not sure that this should be applied. I think it isn't = right
to look at = field is_aborted of parser after parser destruction.

sql_parser_destroy() doesn=E2=80=99t affect = is_abort field. On the other
hand, mb you are right, and using = object after destructor is called
may seem strange. Anyway, = replace pls =E2=80=98if' with ternary operator:
3 lines of = code fit into one.

= --Apple-Mail=_D16210ED-551A-45B4-A7D0-0AFB1EC93DCF--