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