[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Mar 7 20:34:26 MSK 2019
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 *);
>
> /**
More information about the Tarantool-patches
mailing list