[tarantool-patches] Re: [PATCH] sql: fix bug with BLOB TRIM() when X'00' in char set

[n.pettik]

> On 15 Dec 2018, at 13:57, Roman Khabibov <roman.habibov at tarantool.org> wrote:
> 
> 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: ‘… after its’ -> ‘… after it’.

Nit: inculding -> including.

Nit: ‘are then ignored’ -> ‘are ignored’.

Nit: I wouldn’t mix first and second conditions:
‘If the char set contains …, all characters are ignored...'

> 
> Closes #3543
> 
> Branch: https://github.com/tarantool/tarantool/tree/romankhabibov/gh-3543-trim-terminal
> 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(-)
> 
> 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 = zCharSet, nChar = 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’t come up with better names, then at least
mention in comment what they mean).

> +		int sizeInChar = sqlite3_value_bytes(argv[1]);
> +		int length = 0;

What is the difference between sizeInChar, length and nChar?

> +		z = zCharSet;
> +		nChar = 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 = zCharSet;


> +			zStepBack = z;
> 			SQLITE_SKIP_UTF8(z);
> +			length += z - zStepBack;
> +			nChar++;
> 		}
> 		if (nChar > 0) {
> 			azChar =
> @@ -1235,10 +1243,16 @@ trimFunc(sqlite3_context * context, int argc, sqlite3_value ** argv)
> 				return;
> 			}
> 			aLen = (unsigned char *)&azChar[nChar];
> -			for (z = zCharSet, nChar = 0; *z; nChar++) {
> +			z = zCharSet;
> +			nChar = 0;
> +			length = 0;
> +			while(sizeInChar - length) {
> 				azChar[nChar] = (unsigned char *)z;
> +				zStepBack = z;
> 				SQLITE_SKIP_UTF8(z);
> +				length += z - zStepBack;
> 				aLen[nChar] = (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 = require("sqltester")
> -test:plan(14535)
> +test:plan(14547)
> 
> --!./tcltestrunner.lua
> -- 2001 September 15
> @@ -2100,11 +2100,133 @@ test:do_execsql_test(
>         -- </func-22.22>
>     })
> 
> -- This is to test the deprecated sqlite3_aggregate_count() API.
> --
> --test:do_test(
> --    "func-23.1",
> ---    function()
> +--    function()S

Garbage diff.