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 62A0D29C24 for ; Mon, 25 Mar 2019 11:10:57 -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 ZmeItV6bPWTf for ; Mon, 25 Mar 2019 11:10:57 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 1AF4629B67 for ; Mon, 25 Mar 2019 11:10:57 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string From: "n.pettik" In-Reply-To: Date: Mon, 25 Mar 2019 18:10:55 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: 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 Cc: szudin@tarantool.org > On 15 Mar 2019, at 18:45, Stanislav Zudin = wrote: >=20 > 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 #=E2=80=A6. Or Needed for #=E2=80=A6 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 ] AFAIK we follow policy stating that each separate patch shouldn=E2=80=99t 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(-) >=20 > 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 =3D expr->u.zToken; > assert(z !=3D NULL); > - int c =3D sql_dec_or_hex_to_i64(z, &value); > - if (c =3D=3D 1 || (c =3D=3D 2 && !is_neg) || > - (is_neg && value =3D=3D SMALLEST_INT64)) { > + enum atoi_result c =3D 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. Moreover, instead of storing UINT in INT, I suggest to use fair enum and output param is_uint/is_negative/is_whatever/=E2=80=A6 Finally, please add comment explaining that unsigned value is returned only in case when value can=E2=80=99t be fitted into int64 range. I = don=E2=80=99t see such explanation, so at the first sight code may look weird.=20 > + switch(c) { > + case ATOI_OVERFLOW: > if (sql_strnicmp(z, "0x", 2) =3D=3D 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 =3D c =3D=3D 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; > } > } > } >=20 >=20 > 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 =3D 1; > - u64 u =3D 0; > int neg =3D 0; /* assume positive */ > - int i; > - int c =3D 0; > - int nonNum =3D 0; /* True if input contains UTF16 = with high byte non-zero */ > - const char *zStart; > const char *zEnd =3D z + length; > - incr =3D 1; > + int incr =3D 1; > while (z < zEnd && sqlIsspace(*z)) > z +=3D incr; > - if (z < zEnd) { > - if (*z =3D=3D '-') { > - neg =3D 1; > - z +=3D incr; > - } else if (*z =3D=3D '+') { > - z +=3D incr; > - } > - } > - zStart =3D z; > - /* Skip leading zeros. */ > - while (z < zEnd && z[0] =3D=3D '0') { > + > + if (z >=3D zEnd) > + return ATOI_OVERFLOW; /* invalid format */ It=E2=80=99s not fair to return OVERFLOW result in case of =E2=80=9Cinvali= d format=E2=80=9D. See my comment concerning return values above. > + if (*z =3D=3D '-') { > + neg =3D 1; > z +=3D incr; > } > - for (i =3D 0; &z[i] < zEnd && (c =3D z[i]) >=3D '0' && c <=3D = '9'; > - i +=3D incr) { > - u =3D u * 10 + c - '0'; > - } > - if (u > LARGEST_INT64) { > - *val =3D neg ? SMALLEST_INT64 : LARGEST_INT64; > - } else if (neg) { > - *val =3D -(i64) u; > + > + char* end =3D NULL; According to docs, you should set errno to 0 before calling this function. > + u64 u =3D strtoull(z, &end, 10); > + if (end =3D=3D z) > + return ATOI_OVERFLOW; In this case, it=E2=80=99s not overflow but error like =E2=80=9C"No = digits were found=E2=80=9D. > + if (errno =3D=3D 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 =3D=3D ERANGE && (val =3D=3D LONG_MAX || val =3D=3D = LONG_MIN)) || (errno !=3D 0 && val =3D=3D 0)) If these error should be caught before, then add assertions pls.