[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 *);
>   
>