[tarantool-patches] Re: [PATCH v2 0/7] sql: store regular identifiers in case-normal form

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

I see, that you've added a new patch, but decided not to
send it. I've done that. Below you can find my 2 comments on
it.

> commit 47f6bd7a079c8a5af73abe88d9abc24e4bf95f75
> Author: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date:   Mon Mar 11 14:33:57 2019 +0300
> 
>     sql: rework sqlIdListAppend to set diag
>     
>     Refactored sqlIdListAppend routine as sql_id_list_append and
>     reworked it to use diag_set in case of memory allocation error.
>     This change is necessary because the sql_id_list_append body has
>     a sqlNameFromToken call that will be changed in subsequent
>     patches.
>     
>     This patch refers to a series of preparatory patches that provide
>     the use of Tarantool errors in the call tree that includes
>     sqlNormalizeName, since this call can later return errors.
>     
>     This patch is not self-sufficient, its sqlNameFromToken call
>     remained to be non-Tarantool (for now). It means, that if
>     sqlNameFromToken fails in sql_id_list_append there is no
>     diag message created.
>     
>     Part of #3931
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 122fda2f0..8a8f1b82f 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -797,10 +797,17 @@ insert_cmd(A) ::= REPLACE.            {A = ON_CONFLICT_ACTION_REPLACE;}
>  
>  idlist_opt(A) ::= .                       {A = 0;}
>  idlist_opt(A) ::= LP idlist(X) RP.    {A = X;}
> -idlist(A) ::= idlist(A) COMMA nm(Y).
> -    {A = sqlIdListAppend(pParse->db,A,&Y);}
> -idlist(A) ::= nm(Y).
> -    {A = sqlIdListAppend(pParse->db,0,&Y); /*A-overwrites-Y*/}
> +idlist(A) ::= idlist(A) COMMA nm(Y). {
> +  A = sql_id_list_append(pParse->db,A,&Y);
> +  if (A == NULL)
> +    sql_parser_error(pParse);
> +}
> +idlist(A) ::= nm(Y). {
> +  /* A-overwrites-Y. */
> +  A = sql_id_list_append(pParse->db,0,&Y);
> +  if (A == NULL)
> +    sql_parser_error(pParse);

1. Why do not you return after an error?

> +}
>  
>  /////////////////////////// Expression Processing /////////////////////////////
>  //
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2df1830d6..56ae90a95 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3388,7 +3388,20 @@ sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
>  void sqlInsert(Parse *, SrcList *, Select *, IdList *,
>  	       enum on_conflict_action);
>  void *sqlArrayAllocate(sql *, void *, int, int *, int *);
> -IdList *sqlIdListAppend(sql *, IdList *, Token *);
> +
> +/**
> + * Append a new element to the given IdList. Create a new IdList
> + * if need be.
> + *
> + * @param db The database connection.
> + * @param list The pointer to existent Id list if exists.
> + * @param name_token The token containing name.
> + * @retval Not NULL IdList pointer is returned on success.
> + * @retval NULL otherwise. Diag message is set.
> + */
> +struct IdList *
> +sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken);

2. The arg names are old, and do not match the implementation
and the comment.

> +
>  int sqlIdListIndex(IdList *, const char *);
>  
>  /**