Re: [Tarantool-patches] [PATCH v1 01/21] sql: refactor CHAR() function

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the fixes!

See 3 comments below.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index afe34f7f0..dee28b852 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -717,6 +717,57 @@ func_substr_characters(struct sql_context *ctx, int argc, struct Mem *argv)
>  		ctx->is_aborted = true;
>  }
>  
> +/**
> + * Implementation of the CHAR() function.
> + *
> + * This function takes zero or more arguments, each of which is an integer. It
> + * constructs a string where each character of the string is the unicode
> + * character for the corresponding integer argument.
> + *
> + * If an argument is negative or greater than 0x10ffff, the symbol "�" is used.
> + * Symbol '\0' used instead of NULL argument.
> + */
> +static void
> +func_char(struct sql_context *ctx, int argc, struct Mem *argv)
> +{
> +	if (argc == 0)
> +		return mem_set_str_static(ctx->pOut, "", 0);
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
> +	UChar32 *buf = region_alloc(region, argc * sizeof(*buf));

1. Would be better to use region_alloc_array(). Otherwise you
risk to get misaligned data.

> +	if (buf == NULL) {
> +		ctx->is_aborted = true;

2. Need to use diag_set() here.

> +		return;
> +	}
> +	int len = 0;
> +	for (int i = 0; i < argc; ++i) {
> +		if (mem_is_null(&argv[i]))
> +			buf[i] = 0;
> +		else if (!mem_is_uint(&argv[i]) || argv[i].u.u > 0x10ffff)
> +			buf[i] = 0xfffd;
> +		else
> +			buf[i] = argv[i].u.u;
> +		len += U8_LENGTH(buf[i]);
> +	}
> +
> +	char *str = sqlDbMallocRawNN(sql_get(), len);
> +	if (str == NULL) {
> +		ctx->is_aborted = true;

3. region leaks here, doesn't it?