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 3FB9E294E4 for ; Mon, 1 Apr 2019 16:39:09 -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 9SDwrL2HWdzE for ; Mon, 1 Apr 2019 16:39:09 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 E2F842623B for ; Mon, 1 Apr 2019 16:39:08 -0400 (EDT) From: Stanislav Zudin Subject: [tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string References: Message-ID: Date: Mon, 1 Apr 2019 23:39:06 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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, "n.pettik" On 25.03.2019 18:10, n.pettik wrote: > > >> On 15 Mar 2019, at 18:45, Stanislav Zudin wrote: >> >> Correctly handles the signed and unsigned integers on the way from sql >> expression to VDBE. >> VDBE doesn't distinguish yet signed and unsigned integers. > > Firstly, please add tag to each patch related to patch-set: > > Part of #…. > > Or > > Needed for #… Got it. > > Secondly, several tests fail on this patch: > Failed tasks: > - [sql-tap/cast.test.lua, memtx] > - [sql-tap/tkt-a8a0d2996a.test.lua, memtx] > - [sql-tap/misc3.test.lua, memtx] > - [sql-tap/in4.test.lua, memtx] > > sql/gh-2347-max-int-literals.test.lua vinyl [ fail ] > The final commit does pass these tests. > AFAIK we follow policy stating that each separate patch > shouldn’t break existing tests. Mb you should reconsider > structure of your patch-set. > >> src/box/sql/expr.c | 23 ++++--- >> src/box/sql/main.c | 2 +- >> src/box/sql/sqlInt.h | 63 +++++++++++------ >> src/box/sql/util.c | 160 +++++++++++++++++++------------------------ >> src/box/sql/vdbe.c | 2 +- >> 5 files changed, 125 insertions(+), 125 deletions(-) >> >> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >> index 82688dff3..3fbfab7a0 100644 >> --- a/src/box/sql/expr.c >> +++ b/src/box/sql/expr.c >> @@ -3335,23 +3335,24 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg, >> int64_t value; >> const char *z = expr->u.zToken; >> assert(z != NULL); >> - int c = sql_dec_or_hex_to_i64(z, &value); >> - if (c == 1 || (c == 2 && !is_neg) || >> - (is_neg && value == SMALLEST_INT64)) { >> + enum atoi_result c = sql_dec_or_hex_to_i64(z, is_neg, &value); > > I suggest to make all atoi return -1 in case of errors (including overflow), > and 0 in case of successful conversion. Error itself you can set via diag_set(). > Sign of the result you can pass via output param. > This way would be more Tarantool-like. Accepted. Fixed. > > Moreover, instead of storing UINT in INT, I suggest to use fair enum and > output param is_uint/is_negative/is_whatever/… > > Finally, please add comment explaining that unsigned value is returned > only in case when value can’t be fitted into int64 range. I don’t see > such explanation, so at the first sight code may look weird. Done. BTW. The declaration of atoi* function contains such explanation > >> + switch(c) { >> + case ATOI_OVERFLOW: >> if (sql_strnicmp(z, "0x", 2) == 0) { >> sqlErrorMsg(parse, >> - "hex literal too big: %s%s", >> - is_neg ? "-" : "", z); >> + "hex literal too big: %s%s", >> + is_neg ? "-" : "", z); >> } else { >> sqlErrorMsg(parse, >> - "oversized integer: %s%s", >> - is_neg ? "-" : "", z); >> + "oversized integer: %s%s", >> + is_neg ? "-" : "", z); >> } >> - } else { >> - if (is_neg) >> - value = c == 2 ? SMALLEST_INT64 : -value; >> + break; >> + case ATOI_UNSIGNED: >> + case ATOI_SIGNED: >> sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0, >> - (u8 *)&value, P4_INT64); >> + (u8 *)&value, P4_INT64); >> + break; >> } >> } >> } >> >> >> index c89e2e8ab..be77f72f8 100644 >> --- a/src/box/sql/util.c >> +++ b/src/box/sql/util.c >> @@ -591,110 +591,56 @@ sqlAtoF(const char *z, double *pResult, int length) >> #endif /* SQL_OMIT_FLOATING_POINT */ >> -int >> +enum atoi_result >> sql_atoi64(const char *z, int64_t *val, int length) >> { >> - int incr = 1; >> - u64 u = 0; >> int neg = 0; /* assume positive */ >> - int i; >> - int c = 0; >> - int nonNum = 0; /* True if input contains UTF16 with high byte non-zero */ >> - const char *zStart; >> const char *zEnd = z + length; >> - incr = 1; >> + int incr = 1; >> while (z < zEnd && sqlIsspace(*z)) >> z += incr; >> - if (z < zEnd) { >> - if (*z == '-') { >> - neg = 1; >> - z += incr; >> - } else if (*z == '+') { >> - z += incr; >> - } >> - } >> - zStart = z; >> - /* Skip leading zeros. */ >> - while (z < zEnd && z[0] == '0') { >> + >> + if (z >= zEnd) >> + return ATOI_OVERFLOW; /* invalid format */ > > It’s not fair to return OVERFLOW result in case of “invalid format”. > See my comment concerning return values above. > >> + if (*z == '-') { >> + neg = 1; >> z += incr; >> } >> - for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9'; >> - i += incr) { >> - u = u * 10 + c - '0'; >> - } >> - if (u > LARGEST_INT64) { >> - *val = neg ? SMALLEST_INT64 : LARGEST_INT64; >> - } else if (neg) { >> - *val = -(i64) u; >> + >> + char* end = NULL; > > According to docs, you should set errno to 0 before > calling this function. Good point. Fixed. > >> + u64 u = strtoull(z, &end, 10); >> + if (end == z) >> + return ATOI_OVERFLOW; > > In this case, it’s not overflow but error like “"No digits were found”. > >> + if (errno == ERANGE) >> + return ATOI_OVERFLOW; > > Not only ERANGE errors can occur. > Correct way of testing on occurred errors is shown here: > https://linux.die.net/man/3/strtol > > if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) > || (errno != 0 && val == 0)) IMO the (errno != 0) covers all faulty cases. > > If these error should be caught before, then add assertions pls. > >