[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag

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

See 5 comments.

On 27/02/2019 14:13, Kirill Shcherbatov wrote:
> Refactored sql_name_from_token routine to use diag_set in case
> of memory allocation error.
> This change is necessary because the sql_name_from_token body has
> a sqlNameFromToken call that will be changed in subsequent
> patches.
> 
> Needed for #3931
> ---
>   src/box/sql/alter.c   |   8 +-
>   src/box/sql/analyze.c |  47 ++++----
>   src/box/sql/build.c   | 241 +++++++++++++++++++++++-------------------
>   src/box/sql/expr.c    |   2 +-
>   src/box/sql/parse.y   |  15 ++-
>   src/box/sql/pragma.c  |  24 +++--
>   src/box/sql/sqlInt.h  |  35 +++++-
>   src/box/sql/trigger.c |   4 +-
>   8 files changed, 228 insertions(+), 148 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 4fe838608..2084dbfeb 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -332,7 +320,10 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr)
>   		goto cleanup;
>   	sqlVdbeCountChanges(v);
>   
> -	zName = sqlNameFromToken(db, pName);
> +	assert(pName != NULL);

1. You already have the same assertion in sql_name_from_token.
Why do you need so many new equal assertions? Check other places
like build.c:1792 by yourself, please.

> +	zName = sql_name_from_token(db, pName);
> +	if (zName == NULL)
> +		goto tnt_error;
>   
>   	pParse->sNameToken = *pName;
>   	if (zName == 0)

2. Double check for zName == NULL.

> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index a7224f5c2..1e4ba3518 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -452,19 +452,27 @@ sqlPragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>   	sqlVdbeRunOnlyOnce(v);
>   	pParse->nMem = 2;
>   
> -	zLeft = sqlNameFromToken(db, pId);
> -	if (!zLeft) {
> +	if (pId == NULL) {
>   		printActivePragmas(user_session);
>   		return;
>   	}
> -
> +	zLeft = sql_name_from_token(db, pId);
> +	if (zLeft == NULL)
> +		goto tnt_error;
>   	if (minusFlag) {
>   		zRight = sqlMPrintf(db, "-%T", pValue);
>   	} else {
> -		zRight = sqlNameFromToken(db, pValue);
> +		if (pValue != NULL) {

3. You can make it shorter writing

     } else if (pValue != NULL) {
         ...
     }

Instead of

     } else {
         if (pValue != NULL) {
             ...
         }
     }

> +			zRight = sql_name_from_token(db, pValue);
> +			if (zRight == NULL)
> +				goto tnt_error;
> +		}
> +	}
> +	if (pValue2 != NULL) {
> +		zTable = sql_name_from_token(db, pValue2);
> +		if (zTable == NULL)
> +			goto tnt_error;
>   	}
> -	zTable = sqlNameFromToken(db, pValue2);
> -
>   	/* Locate the pragma in the lookup table */
>   	pPragma = pragmaLocate(zLeft);
>   	if (pPragma == 0) {
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index eae2ec8e8..10943d3c8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3424,7 +3424,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.
> + * A new IdList is returned, or NULL if malloc() fails.

4. The last sentence is exactly what you wrote after @retvals.
Please, do not duplicate it.

> + * @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.

5. Sentences are started from capital letters. I will not write
that again. Please, find and fix other places.

> + */
> +struct IdList *
> +sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken);
> +
>   int sqlIdListIndex(IdList *, const char *);
>   
>   /**