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 33246262CA for ; Thu, 24 Jan 2019 13:29:15 -0500 (EST) 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 I17KgcjjCVaR for ; Thu, 24 Jan 2019 13:29:15 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 DED11262B9 for ; Thu, 24 Jan 2019 13:29:14 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/2] sql: refactor sql_expr_coll and sql_binary_compare_coll_seq functions References: <276917ac3b405134ae0f075127888912395d511e.1547644179.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <07f58a45-5cf2-e848-78f1-c8a67028733c@tarantool.org> Date: Thu, 24 Jan 2019 21:28:59 +0300 MIME-Version: 1.0 In-Reply-To: <276917ac3b405134ae0f075127888912395d511e.1547644179.git.korablev@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, Nikita Pettik Hi! Thanks for the patch! On 16/01/2019 16:13, Nikita Pettik wrote: > Lets make sql_expr_coll() return error code for two reasons. Firstly, > we are going to use this function to detect operands of concatenation > with incompatible collations. Secondly, pointer to struct coll in most > cases is redundant since collation id (which in turn returned via output > parameter) is enough to proceed the same operations. > For the same reason lets make sql_binary_compare_coll_seq() return > collation id instead of struct coll* and remove corresponding output > parameter. First of all, I agree with Kostja - it does not help in anything that you removed coll * from output parameters/return value. It just slows down some places like codeCompare, sqlite3ExprCodeIN, sqlite3ExprCodeTarget etc. - you just do coll_by_id() twice. Moreover, maybe making some places more accurate and compact, it pads out other ones and involves additional headers like in whereexpr.c. Please, return as it was. Nevertheless, see 4 comments below. > > Needed for #3947 > --- > src/box/sql/expr.c | 82 ++++++++++++++++++++++++------------------------- > src/box/sql/select.c | 27 ++++++++++------ > src/box/sql/sqliteInt.h | 14 ++++----- > src/box/sql/where.c | 56 ++++++++++++++++----------------- > src/box/sql/whereexpr.c | 23 ++++++++------ > 5 files changed, 105 insertions(+), 97 deletions(-) > > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index b67b22c23..f8819f779 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -205,7 +204,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) > } > if (op == TK_COLLATE || > (op == TK_REGISTER && p->op2 == TK_COLLATE)) { > - coll = sql_get_coll_seq(parse, p->u.zToken, coll_id); > + (void) sql_get_coll_seq(parse, p->u.zToken, coll_id); 1. It is not necessary to prefix (void) each function result of which you ignore. By the way, I do not understand why do you ignore it. Here and in all other places. > *is_explicit_coll = true; > break; > } > @@ -377,27 +376,28 @@ illegal_collation_mix: > return -1; > } > > -struct coll * > -sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, > - uint32_t *coll_id) > +uint32_t > +sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right) > { > assert(left != NULL); > bool is_lhs_forced; > bool is_rhs_forced; > uint32_t lhs_coll_id; > uint32_t rhs_coll_id; > - struct coll *lhs_coll = sql_expr_coll(parser, left, &is_lhs_forced, > - &lhs_coll_id); > - struct coll *rhs_coll = sql_expr_coll(parser, right, &is_rhs_forced, > - &rhs_coll_id); > + if (sql_expr_coll(parser, left, &is_lhs_forced, &lhs_coll_id) != 0) > + goto err; > + if (sql_expr_coll(parser, right, &is_rhs_forced, &rhs_coll_id) != 0) > + goto err; > + uint32_t coll_id; > if (collations_check_compatibility(lhs_coll_id, is_lhs_forced, > rhs_coll_id, is_rhs_forced, > - coll_id) != 0) { > - parser->rc = SQL_TARANTOOL_ERROR; > - parser->nErr++; > - return NULL; > - } > - return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;; > + &coll_id) != 0) > + goto err; > + return coll_id; > +err: > + parser->rc = SQL_TARANTOOL_ERROR; > + parser->nErr++; > + return COLL_NONE; 2. Please, do not return some obscure error codes - we use only 0/-1 and NULL/not NULL almost everywhere except some archaic or misreviewed code. If you want to return an error, it is better to move coll_id to out parameters. > } > > /* > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 7501fadc8..50d297815 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -4337,10 +4337,10 @@ const char *sqlite3ErrStr(int); > * clause is used. > * @param[out] coll_id Collation identifier. > * > - * @retval Pointer to collation. > + * @retval Return code: < 0 in case of error, 0 on success. 3. Please, use separate @retval for each value. > */ > -struct coll * > -sql_expr_coll(Parse * pParse, Expr * pExpr, bool *is_explicit_coll, > +int > +sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, > uint32_t *coll_id); > > Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int); > @@ -4684,13 +4684,11 @@ collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced, > * @param parser Parser. > * @param left Left expression. > * @param right Right expression. Can be NULL. > - * @param[out] coll_id Collation identifier. > * > - * @retval Collation object. > + * @retval Id of collation object. 4. You didn't mentioned that it can be COLL_NONE, that means an error. > */ > -struct coll * > -sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, > - uint32_t *coll_id); > +uint32_t > +sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right); > int sqlite3TempInMemory(const sqlite3 *); > #ifndef SQLITE_OMIT_CTE > With *sqlite3WithAdd(Parse *, With *, Token *, ExprList *, Select *);