[Tarantool-patches] [PATCH v1 2/2] sql: introduce mem_snprintf()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Nov 18 02:03:18 MSK 2021
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?
More information about the Tarantool-patches
mailing list