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 492582266D for ; Tue, 18 Dec 2018 03:40:09 -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 A5ZFYM0g9_zT for ; Tue, 18 Dec 2018 03:40:09 -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 EF6A82232F for ; Tue, 18 Dec 2018 03:40:08 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH] sql: fix bug with BLOB TRIM() when X'00' in char set From: "n.pettik" In-Reply-To: <20181215105741.28464-1-roman.habibov@tarantool.org> Date: Tue, 18 Dec 2018 11:40:05 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20181215105741.28464-1-roman.habibov@tarantool.org> 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: Roman Khabibov > On 15 Dec 2018, at 13:57, Roman Khabibov = wrote: >=20 > The reason for the bug was that X'00' is a terminal symbol. If the = char set > contained X'00', all other characters after its (inculding itself) are = then > ignored. Nit: =E2=80=98=E2=80=A6 after its=E2=80=99 -> =E2=80=98=E2=80=A6 after = it=E2=80=99. Nit: inculding -> including. Nit: =E2=80=98are then ignored=E2=80=99 -> =E2=80=98are ignored=E2=80=99. Nit: I wouldn=E2=80=99t mix first and second conditions: =E2=80=98If the char set contains =E2=80=A6, all characters are = ignored...' >=20 > Closes #3543 >=20 > Branch: = https://github.com/tarantool/tarantool/tree/romankhabibov/gh-3543-trim-ter= minal > Issue: https://github.com/tarantool/tarantool/issues/3543 You should put links to the branch and issue below delimiter ---, not = above. > --- > src/box/sql/func.c | 18 +++++- > test/sql-tap/func.test.lua | 126 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 140 insertions(+), 4 deletions(-) >=20 > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 9667aead5..5beba7bd2 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -1224,8 +1224,16 @@ trimFunc(sqlite3_context * context, int argc, = sqlite3_value ** argv) > return; > } else { > const unsigned char *z; > - for (z =3D zCharSet, nChar =3D 0; *z; nChar++) { Lets start from putting here comments which explain what is going on here (how you algorithm/approach works). Then, lets choose more suitable names for variables: length, sizeInChar and nChar sound very similar (if you can=E2=80=99t come up with better names, then at least mention in comment what they mean). > + int sizeInChar =3D sqlite3_value_bytes(argv[1]); > + int length =3D 0; What is the difference between sizeInChar, length and nChar? > + z =3D zCharSet; > + nChar =3D 0; > + const unsigned char *zStepBack; > + while(sizeInChar - length) { We use explicit conditions as a predicate value. In other words: while (sizeInChar - length > 0) Also, it would be better if you declare and initialise var at once: const unsigned char *z =3D zCharSet; > + zStepBack =3D z; > SQLITE_SKIP_UTF8(z); > + length +=3D z - zStepBack; > + nChar++; > } > if (nChar > 0) { > azChar =3D > @@ -1235,10 +1243,16 @@ trimFunc(sqlite3_context * context, int argc, = sqlite3_value ** argv) > return; > } > aLen =3D (unsigned char *)&azChar[nChar]; > - for (z =3D zCharSet, nChar =3D 0; *z; nChar++) { > + z =3D zCharSet; > + nChar =3D 0; > + length =3D 0; > + while(sizeInChar - length) { > azChar[nChar] =3D (unsigned char *)z; > + zStepBack =3D z; > SQLITE_SKIP_UTF8(z); > + length +=3D z - zStepBack; > aLen[nChar] =3D (u8) (z - = azChar[nChar]); > + nChar++; > } > } > } > diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua > index 393212968..b7b8e7c4c 100755 > --- a/test/sql-tap/func.test.lua > +++ b/test/sql-tap/func.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(14535) > +test:plan(14547) >=20 > --!./tcltestrunner.lua > -- 2001 September 15 > @@ -2100,11 +2100,133 @@ test:do_execsql_test( > -- > }) >=20 > -- This is to test the deprecated sqlite3_aggregate_count() API. > -- > --test:do_test( > -- "func-23.1", > --- function() > +-- function()S Garbage diff.