[tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2748 bytes --]


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


[-- Attachment #2: Type: text/html, Size: 9343 bytes --]