Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce mem_snprintf()

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

Hi! Thanks for the patch!

See 5 comments below.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 5abaf490d..0f8f0b5cc 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -869,9 +869,14 @@ func_printf(struct sql_context *ctx, int argc, const struct Mem *argv)
>  	if (argc < 1 || mem_is_null(&argv[0]))
>  		return;
>  	if (argc == 1 || !mem_is_str(&argv[0])) {
> -		struct Mem *mem = ctx->pOut;
> -		if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0)
> +		uint32_t size = mem_snprintf(NULL, 0, &argv[0]);

1. You are not checking for an error here. mem_snprintf() returns int,
not uint. I am fine with making it never failing, but then we need
to at least add some assertions inside of mem_snprintf() that its
internal snprintf() calls never fail. We can keep returning 'int' though
because it makes the function compatible with SNPRINT macro.

> +		char *str = sqlDbMallocRawNN(sql_get(), size + 1);
> +		if (str == NULL) {
>  			ctx->is_aborted = true;
> +			return;
> +		}
> +		mem_snprintf(str, size + 1, &argv[0]);
> +		mem_set_str_allocated(ctx->pOut, str, size);
>  		return;
>  	}
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 9ddeea5bb..6e1729f32 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -116,30 +116,51 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type)
>  	return field_mp_plain_type_is_compatible(type, mp_type, true);
>  }
>  
> -const char *
> -mem_str(const struct Mem *mem)
> +int
> +mem_snprintf(char *buf, uint32_t size, const struct Mem *mem)
>  {
> -	char buf[STR_VALUE_MAX_LEN];
> -	const char *type = mem_type_to_str(mem);
>  	switch (mem->type) {
>  	case MEM_TYPE_NULL:
> -		return "NULL";
> +		return snprintf(buf, size, "NULL");
>  	case MEM_TYPE_STR:
> +	case MEM_TYPE_BIN:
> +		return snprintf(buf, size, "%.*s", mem->n, mem->z);

2. Why is bin displayed as hex in mem_str() but as a plain string here?

> +	case MEM_TYPE_INT:
> +		return snprintf(buf, size, "%lld", mem->u.i);
> +	case MEM_TYPE_UINT:
> +		return snprintf(buf, size, "%llu",
> +				(unsigned long long)mem->u.u);
> +	case MEM_TYPE_DOUBLE: {
> +		char str[BUF_SIZE];
> +		sql_snprintf(BUF_SIZE, str, "%!.15g", mem->u.r);
> +		return snprintf(buf, size, "%s", str);
> +	}
> +	case MEM_TYPE_DEC:
> +		return snprintf(buf, size, "%s", decimal_str(&mem->u.d));
> +	case MEM_TYPE_MAP:
> +	case MEM_TYPE_ARRAY:
> +		return mp_snprint(buf, size, mem->z);
> +	case MEM_TYPE_UUID:
> +		return snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid));
> +	case MEM_TYPE_BOOL:
> +		return snprintf(buf, size, "%s", mem->u.b ? "TRUE" : "FALSE");

3. You can drop '%s' here:

	return snprintf(buf, size, mem->u.b ? "TRUE" : "FALSE");

> +	default:
> +		return snprintf(buf, size, "unknown");

4. Maybe add an assertion that it is not possible? We don't have non-typed
mems AFAIK.

> +	}
> +}
> diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c
> index 8da7c9878..08e18c15d 100644
> --- a/src/box/sql/printf.c
> +++ b/src/box/sql/printf.c
> @@ -160,19 +160,12 @@ getTextArg(PrintfArguments * p)
>  {
>  	if (p->nArg <= p->nUsed)
>  		return 0;
> -	struct Mem mem;
> -	mem_create(&mem);
> -	mem_copy_as_ephemeral(&mem, &p->apArg[p->nUsed++]);
> -	if (mem_to_str(&mem) != 0) {
> -		mem_destroy(&mem);
> -		return NULL;
> -	}
> -	char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1);
> +	const struct Mem *mem = &p->apArg[p->nUsed++];
> +	uint32_t size = mem_snprintf(NULL, 0, mem);
> +	char *str = sqlDbMallocRawNN(sql_get(), size + 1);
>  	if (str == NULL)
>  		return NULL;
> -	memcpy(str, mem.z, mem.n);
> -	str[mem.n] = '\0';
> -	mem_destroy(&mem);
> +	mem_snprintf(str, size + 1, mem);

5. Hm. I suspect this pattern of snprintf + malloc + snprintf
is going to be frequent. Maybe wrap it into a function like
mem_strdup()? It would do these 3 actions inside. WDYT?