Re: [Tarantool-patches] [PATCH v4 08/53] sql: introduce mem_str()

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

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 62338e1db..0ed5e38d4 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -37,6 +37,39 @@
>  #include "box/tuple.h"
>  #include "mpstream/mpstream.h"
>  
> +enum {
> +	BUF_SIZE = 32,
> +};
> +
> +const char *
> +mem_str(const struct Mem *mem)
> +{
> +	char buf[BUF_SIZE];
> +	switch (mem->flags & MEM_PURE_TYPE_MASK) {
> +	case MEM_Null:
> +		return "NULL";
> +	case MEM_Str:
> +		return tt_sprintf("%.*s", mem->n, mem->z);
> +	case MEM_Int:
> +		return tt_sprintf("%lld", mem->u.i);
> +	case MEM_UInt:
> +		return tt_sprintf("%llu", mem->u.u);
> +	case MEM_Real:
> +		sql_snprintf(BUF_SIZE, &buf[0], "%!.15g", mem->u.r);
> +		return tt_sprintf("%s", buf);
> +	case MEM_Blob:
> +		if ((mem->flags & MEM_Subtype) == 0)
> +			return "varbinary";
> +		assert(mem->subtype == SQL_SUBTYPE_MSGPACK);
> +		return mp_str(mem->z);
> +	case MEM_Bool:
> +		return mem->u.b ? "TRUE" : "FALSE";

1. Why are they capital?

> +	default:
> +		break;
> +	}
> +	return "unknown";
> +}
> diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
> index e84bb3192..eceaa953b 100644
> --- a/src/box/sql/vdbetrace.c
> +++ b/src/box/sql/vdbetrace.c
> @@ -147,33 +145,10 @@ sqlVdbeExpandSql(Vdbe * p,	/* The prepared statement being evaluated */
>  			zRawSql += nToken;
>  			nextIndex = idx + 1;
>  			assert(idx > 0 && idx <= p->nVar);
> -			pVar = &p->aVar[idx - 1];
> -			if (pVar->flags & MEM_Null) {
> -				sqlStrAccumAppend(&out, "NULL", 4);
> -			} else if (pVar->flags & MEM_Int) {
> -				sqlXPrintf(&out, "%lld", pVar->u.i);
> -			} else if (pVar->flags & MEM_UInt) {
> -				sqlXPrintf(&out, "%llu", pVar->u.u);
> -			} else if (pVar->flags & MEM_Real) {
> -				sqlXPrintf(&out, "%!.15g", pVar->u.r);
> -			} else if (pVar->flags & MEM_Str) {
> -				int nOut;	/* Number of bytes of the string text to include in output */
> -				nOut = pVar->n;
> -				sqlXPrintf(&out, "'%.*q'", nOut, pVar->z);
> -			} else if (pVar->flags & MEM_Zero) {
> -				sqlXPrintf(&out, "zeroblob(%d)",
> -					       pVar->u.nZero);
> -			} else {
> -				int nOut;	/* Number of bytes of the blob to include in output */
> -				assert(pVar->flags & MEM_Blob);
> -				sqlStrAccumAppend(&out, "x'", 2);
> -				nOut = pVar->n;
> -				for (i = 0; i < nOut; i++) {
> -					sqlXPrintf(&out, "%02x",
> -						       pVar->z[i] & 0xff);
> -				}
> -				sqlStrAccumAppend(&out, "'", 1);
> -			}
> +			const char *value = mem_str(&p->aVar[idx - 1]);
> +			uint32_t size = MIN(strlen(value), sizeof(zBase) - 1);

2. strlen() might be called twice because MIN is a macro.

> +			memcpy(zBase, value, size);
> +			zBase[size] = '\0';
>  		}
>  	}
>  	if (out.accError)