[tarantool-patches] Re: [PATCH v1 1/2] sql: get rid off sqlite3NestedParse in clean stats
- From: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>
- To: tarantool-patches@xxxxxxxxxxxxx
- Date: Fri, 6 Jul 2018 21:13:22 +0300
I have pushed my review fixes on the branch. Please, squash, if
you agree, and debug if the tests fail. Below you may find 3
comments that I fixed. Style violations I did not mentioned here.
Just fixed on the branch.
Hello! Thank you for so detailed review fixes.
I've squashed your changes, they are looking good for me and they also pass all
tests.
Also I have found that vdbe_emit_open_cursor() has the second
parameter named 'index_id', but in some places the function
takes real index_id, in other places it takes tnum, and in vdbe
it is interpreted as tnum. Please, fix this mess in a separate
commit. I think, we should always pass index_id.
I've tried to do this and I'll append my diff to the end of this message,
but there are some strange things..
This should be investigated separately. I don't mind to do this, but I believe
that this may delayed to be not a part of "remove sqlite3NestedParse function".
Take a look at the end of this message.
1. Unused array.
2. You do not need to lookup pStat for tnum in non-debug build since
you know stat tables id from schema_def.h. This HashFind is useful for
debug assertion only.
You've fixed all of this.
+ struct Expr *col_eq_expr =
+ sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr);
+ if (col_type_expr == NULL || col_name_expr == NULL) {
3. If col_eq_expr == NULL here, then col_type/name_expr leak.
As you already coincide (according to you changes on branch), there is no
leak as sqlite3PExpr manually releases arguments on failure.
----------------------------------
Other related posts:
