[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 24 22:26:40 MSK 2018
Thanks for the patch! See 6 comments below.
On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> As we would store Check source expr in MsgPack we need
> span instead of parsed Expr only.
> Renamed refactored function to sql_add_check_constraint.
1. Renamed refactored?
>
> Part of #3272.
> ---
> src/box/sql/build.c | 24 ++++++++++--------------
> src/box/sql/parse.y | 4 ++--
> src/box/sql/sqliteInt.h | 9 ++++++++-
> 3 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a6ddcf0..afb1128 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1019,26 +1019,22 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */
> return;
> }
>
> -/*
> - * Add a new CHECK constraint to the table currently under construction.
> - */
> void
> -sqlite3AddCheckConstraint(Parse * pParse, /* Parsing context */
> - Expr * pCheckExpr /* The check expression */
> - )
> +sql_add_check_constraint(Parse *parser, ExprSpan *span)
> {
> #ifndef SQLITE_OMIT_CHECK
2. This macro is not defined. You can remove it.
> - Table *pTab = pParse->pNewTable;
> - if (pTab) {
> - pTab->pCheck =
> - sqlite3ExprListAppend(pParse, pTab->pCheck, pCheckExpr);
> - if (pParse->constraintName.n) {
> - sqlite3ExprListSetName(pParse, pTab->pCheck,
> - &pParse->constraintName, 1);
> + struct Expr *expr = span->pExpr;
> + Table *table = parser->pNewTable;
> + if (table != NULL) {
> + table->pCheck =
> + sqlite3ExprListAppend(parser, table->pCheck, expr);
> + if (parser->constraintName.n) {
3. != 0
> + sqlite3ExprListSetName(parser, table->pCheck,
> + &parser->constraintName, 1);
> }
> } else
> #endif
> - sql_expr_delete(pParse->db, pCheckExpr, false);
> + sql_expr_delete(parser->db, expr, false);
4. If 'if' body is in {}, then else must be too.
> }
>
> /*
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index a7ef80f..a1a5269 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3532,7 +3532,14 @@ void sqlite3StartTable(Parse *, Token *, int);
> void sqlite3AddColumn(Parse *, Token *, Token *);
> void sqlite3AddNotNull(Parse *, int);
> void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);
> -void sqlite3AddCheckConstraint(Parse *, Expr *);
> +
> +/**
> + * Add a new CHECK constraint to the table currently under construction.
> + * @param parser Parsing context.
> + * @param span parser parsed expression object..
5. span parser? And the span is not parsed. You said it in the commit message:
"we need span instead of parsed Expr".
> + */
> +void sql_add_check_constraint(Parse *parser, ExprSpan *span);
6. Wrap line after return type declaration.
> +
> void sqlite3AddDefaultValue(Parse *, ExprSpan *);
> void sqlite3AddCollateType(Parse *, Token *);
>
>
More information about the Tarantool-patches
mailing list