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

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

The diff in your email has nothing to do with sql_name_from_token.
You've not sent a new version of that patch, so I did it below
with my 2 comments inlined.

> commit b052b1b0a43159ac79320d5dca43a201fffa6ab9
> Author: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date:   Wed Feb 13 15:15:22 2019 +0300
> 
>     sql: rework sqlNameFromToken to set diag
>     
>     Refactored sqlNameFromToken routine as sql_name_from_token and
>     reworked it 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.

1. Now, reread that paragraph and fix what is wrong. You do not call
sqlNameFromToken from sql_name_from_token.

>     
>     Part of #3931
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d374acb47..1864bd0ad 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2981,19 +3001,23 @@ sqlWithAdd(Parse * pParse,	/* Parsing context */
>  {
>  	sql *db = pParse->db;
>  	With *pNew;
> -	char *zName;
>  
> -	/* Check that the CTE name is unique within this WITH clause. If
> -	 * not, store an error in the Parse structure.
> +	/*
> +	 * Check that the CTE name is unique within this WITH
> +	 * clause. If not, store an error in the Parse structure.
>  	 */
> -	zName = sqlNameFromToken(pParse->db, pName);
> -	if (zName && pWith) {
> +	char *name = sql_name_from_token(db, pName);
> +	if (name == NULL) {
> +		sql_parser_error(pParse);
> +		return NULL;

2. Leak. sqlWithAdd should delete some structures in a case of OOM.
Lines 3035 - 3038. Also, that function does not return NULL on an
error - it should return the old value. Otherwise you have a second
leak in parse.y:1494.

> +	}
> +	if (pWith != NULL) {
>  		int i;
>  		for (i = 0; i < pWith->nCte; i++) {
> -			if (strcmp(zName, pWith->a[i].zName) == 0) {
> +			if (strcmp(name, pWith->a[i].zName) == 0) {
>  				sqlErrorMsg(pParse,
> -						"duplicate WITH table name: %s",
> -						zName);
> +					    "duplicate WITH table name: %s",
> +					    name);
>  			}
>  		}
>  	}