[tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse
n.pettik
korablev at tarantool.org
Tue Mar 19 12:54:45 MSK 2019
>>>>> 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’t 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”
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 = 0;
>>> - if (parser.rc != SQL_OK) {
>>> - /* Tarantool error may be already set with diag. */
>>> - if (parser.rc != SQL_TARANTOOL_ERROR)
>>> - diag_set(ClientError, ER_SQL, parser.zErrMsg);
>>> + if (parser.is_aborted)
>>> rc = -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 = true;
>>
>> sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, expr_list);
>> - int rc = 0;
>> - if (parser.is_aborted)
>> - rc = -1;
>> sql_parser_destroy(&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’t 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 ‘if' with ternary operator:
3 lines of code fit into one.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190319/406cfd73/attachment.html>
More information about the Tarantool-patches
mailing list