[tarantool-patches] Re: [PATCH v3 05/10] sql: refactor sql_expr_compile to return AST

[Kirill Shcherbatov]

> 1. Here parser.region leaks.
> 2. If a TARANTOOL_ERROR has occurred and parsed_expr had been set before it,
> then you return not NULL, but should return NULL.
> 
> And you should destroy the expression. Looks like it was a leak before your
> patch. Lets always delete all the parsed expressions in parser_destroy (here
> only parsed_expr is). And in this function on success nullify this member
> before destroy.

diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e43fbcb..34ee8e3 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
        }
        parser->disableLookaside = 0;
        sqlite3DbFree(db, parser->zErrMsg);
+       sql_expr_delete(db, parser->parsed_expr, false);
        region_destroy(&parser->region);
 }
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index d6f5a43..9e4576b 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -559,16 +559,20 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
        char *stmt = (char *)region_alloc(&parser.region, len + 1);
        if (stmt == NULL) {
                diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
-               return NULL;
+               goto end;
        }
        sprintf(stmt, "%s%.*s", outer, expr_len, expr);
 
        char *sql_error;
-       if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK &&
-           parser.rc != SQL_TARANTOOL_ERROR)
-               diag_set(ClientError, ER_SQL, sql_error);
-       else
+       if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK) {
+               if (parser.rc != SQL_TARANTOOL_ERROR)
+                       diag_set(ClientError, ER_SQL, sql_error);
+       } else {
                expression = parser.parsed_expr;
+               parser.parsed_expr = NULL;
+       }
+
+end:
        sql_parser_destroy(&parser);
        return expression;
 }