From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0A0F62975F for ; Mon, 18 Mar 2019 15:33:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id z8h89K4WA4-n for ; Mon, 18 Mar 2019 15:33:57 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9210A27BB3 for ; Mon, 18 Mar 2019 15:33:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 6/7] sql: refactor sql_expr_create to set diag References: <645e4a4d0e2ddeaf747fe8a11affb52cb3157df3.1551265819.git.kshcherbatov@tarantool.org> <30714d52-a753-0d00-b28d-d05a1fd2a623@tarantool.org> <23e9426a-d4f3-7954-f342-6a5c738d1cca@tarantool.org> From: Vladislav Shpilevoy Message-ID: <12d01358-2f48-0b26-9560-d7f274de3f0f@tarantool.org> Date: Mon, 18 Mar 2019 22:33:55 +0300 MIME-Version: 1.0 In-Reply-To: <23e9426a-d4f3-7954-f342-6a5c738d1cca@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Thanks for the fixes! See 6 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 44b6ce11f..f5b2926c8 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -149,16 +149,17 @@ sqlExprAddCollateToken(Parse * pParse, /* Parsing context */ > int dequote /* True to dequote pCollName */ > ) > { > - if (pCollName->n > 0) { > - Expr *pNew = > - sqlExprAlloc(pParse->db, TK_COLLATE, pCollName, > - dequote); > - if (pNew) { > - pNew->pLeft = pExpr; > - pNew->flags |= EP_Collate | EP_Skip; > - pExpr = pNew; > - } > + if (pCollName->n == 0) > + return pExpr; > + struct Expr *new_expr = > + sql_expr_new(pParse->db, TK_COLLATE, pCollName, dequote); > + if (new_expr == NULL) { > + sql_parser_error(pParse); > + return NULL; 1. Now you have a leak - parse.y:935 expects that sqlExprAddCollateToken on an error returns the old value, that is freed later, but after your patch the old value is lost. > @@ -854,113 +855,62 @@ sqlExprSetHeightAndFlags(Parse * pParse, Expr * p) > -/* Allocate a new expression and initialize it as integer. > - * @param db sql engine. > - * @param value Value to initialize by. > - * > - * @retval not NULL Allocated and initialized expr. > - * @retval NULL Memory error. > - */ > -Expr * > -sqlExprInteger(sql * db, int value) > +struct Expr * > +sql_op_expr_new(struct sql *db, int op, const char *name) > { > - Expr *ret = sqlExpr(db, TK_INTEGER, NULL); > - if (ret != NULL) { > - ret->flags = EP_IntValue; > - ret->u.iValue = value; > - } > - return ret; > + struct Token name_token; > + name_token.z = name; > + name_token.n = name != NULL ? strlen(name) : 0; 2. sqlTokenInit. > diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c > index d1eb6670a..ceba2db9f 100644 > --- a/src/box/sql/fk_constraint.c > +++ b/src/box/sql/fk_constraint.c > @@ -309,31 +309,30 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent, > * the data for given space. regBase+1 holds the first column. > * regBase+2 holds the second column, and so forth. > * 3. Now the names above are outdated. > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index f0388b481..1a5882dda 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -529,12 +529,16 @@ selcollist(A) ::= sclp(A) expr(X) as(Y). { > sqlExprListSetSpan(pParse,A,&X); > } > selcollist(A) ::= sclp(A) STAR. { > - Expr *p = sqlExpr(pParse->db, TK_ASTERISK, 0); > + struct Expr *p = sql_op_expr_new(pParse->db, TK_ASTERISK, NULL); > + if (p == NULL) > + sql_parser_error(pParse); > A = sql_expr_list_append(pParse->db, A, p); > } > selcollist(A) ::= sclp(A) nm(X) DOT STAR. { > Expr *pRight = sqlPExpr(pParse, TK_ASTERISK, 0, 0); > - Expr *pLeft = sqlExprAlloc(pParse->db, TK_ID, &X, 1); > + struct Expr *pLeft = sql_expr_new(pParse->db, TK_ID, &X, true); > + if (pLeft == NULL) > + sql_parser_error(pParse); 4. The same as about other similar places. Why not 'return'? > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index fda4296cc..5434a2ab0 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c> @@ -506,7 +511,12 @@ addWhereTerm(Parse * pParse, /* Parsing context */ > ExprSetVVAProperty(pEq, EP_NoReduce); > pEq->iRightJoinTable = (i16) pE2->iTable; > } > - *ppWhere = sqlExprAnd(db, *ppWhere, pEq); > + if (*ppWhere != NULL || pEq != NULL) { > + *ppWhere = sql_and_expr_new(pParse->db, *ppWhere, pEq); > + if (*ppWhere == NULL) > + sql_parser_error(pParse); > + } > + return; 5. Why do you need a return at the end of a 'void' function? > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 4e6c5717b..4401ea8b7 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3218,13 +3218,73 @@ void sqlClearTempRegCache(Parse *); > #ifdef SQL_DEBUG > int sqlNoTempsInRange(Parse *, int, int); > #endif > -Expr *sqlExprAlloc(sql *, int, const Token *, int); > -Expr *sqlExpr(sql *, int, const char *); > -Expr *sqlExprInteger(sql *, int); > + > +/** > + * This routine is the core allocator for Expr nodes. > + * Construct a new expression node and return a pointer to it. > + * Memory for this node and for the token argument is a single > + * allocation obtained from sqlDbMalloc(). The calling function > + * is responsible for making sure the node eventually gets freed. > + * > + * If dequote is true, then the token (if it exists) is dequoted. > + * If dequote is false, no dequoting is performed. The dequote > + * parameter is ignored if token is NULL or if the token does > + * not appear to be quoted. If the quotes were of the form "..." > + * (double-quotes) then the EP_DblQuoted flag is set on the > + * expression node. > + * > + * Special case: If op==TK_INTEGER and token points to a string > + * that can be translated into a 32-bit integer, then the token is > + * not stored in u.zToken. Instead, the integer values is written > + * into u.iValue and the EP_IntValue flag is set. No extra storage > + * is allocated to hold the integer text and the dequote flag is > + * ignored. > + * > + * @param db The database connection. > + * @param op Expression opcode (TK_*). > + * @param token Source token. Might be NULL. > + * @param dequote True to dequote string. > + * @retval Not NULL new expression object on success. > + * @retval NULL otherwise. The diag message is set. > + */ > +struct Expr * > +sql_expr_new(struct sql *db, int op, const Token *token, bool dequote); > + > +/** > + * Allocate a new expression node from a zero-terminated token > + * that has already been dequoted. > + * > + * @param db The database connection. > + * @param op Expression opcode. > + * @param name The object name string. > + * @retval Not NULL expression pointer on success. > + * @retval NULL Otherwise. The diag message is set. > + */ > +struct Expr * > +sql_op_expr_new(struct sql *db, int op, const char *name); > + > void sqlExprAttachSubtrees(sql *, Expr *, Expr *, Expr *); > Expr *sqlPExpr(Parse *, int, Expr *, Expr *); > void sqlPExprAddSelect(Parse *, Expr *, Select *); > -Expr *sqlExprAnd(sql *, Expr *, Expr *); > + > +/** > + * Join two expressions using an AND operator. If either > + * expression is NULL, then just return the other expression. > + * > + * If one side or the other of the AND is known to be false, then > + * instead of returning an AND expression, just return a constant > + * expression with a value of false. > + * > + * @param db The database connection. > + * @param left_expr The left-branch expresion to join. > + * @param right_expr The right-branch expression to join. > + * @retval Not NULL new expression root node pointer on success. > + * @retval NULL Otherwise. The diag message is set. > + */ > +struct Expr * > +sql_and_expr_new(struct sql *db, struct Expr *left_expr, > + struct Expr *right_expr); > + > Expr *sqlExprFunction(Parse *, ExprList *, Token *); > void sqlExprAssignVarNumber(Parse *, Expr *, u32); > ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *); > @@ -4732,7 +4792,21 @@ void sqlAppendChar(StrAccum *, int, char); > char *sqlStrAccumFinish(StrAccum *); > void sqlStrAccumReset(StrAccum *); > void sqlSelectDestInit(SelectDest *, int, int, int); > -Expr *sqlCreateColumnExpr(sql *, SrcList *, int, int); > + > +/* > + * Allocate and return a pointer to an expression to load the > + * column from datasource src_idx in SrcList src_list. > + * > + * @param db The database connection. > + * @param src_list The source list described with FROM clause. > + * @param src_idx The resource index to use in src_list. > + * @param column The column index. > + * @retval Not NULL expression pointer on success. 6. I am still not sure, if I said that earlier, but start sentences with a capital letter. It is a simple rule. Here and in all other places below and above.