Re: [Tarantool-patches] [PATCH v5 28/52] sql: introduce mem_copy_str() and mem_copy_str0()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you! I applied your patch. However, I added a bit of changes since it was
possible that after mem_copy_str() type of copied value will be MEM_Blob.

Diff:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index d98b077ad..7676a29e3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -258,7 +258,11 @@ mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
 {
 	if ((mem->flags & (MEM_Str | MEM_Blob)) != 0 && mem->z == value) {
 		/* Own value, but might be ephemeral. Make it own if so. */
-		return sqlVdbeMemGrow(mem, len, 1);
+		if (sqlVdbeMemGrow(mem, len, 1) != 0)
+			return -1;
+		mem->flags = MEM_Str;
+		mem->field_type = FIELD_TYPE_STRING;
+		return 0;
 	}
 	mem_clear(mem);
 	if (sqlVdbeMemGrow(mem, len, 0) != 0)


On Thu, Apr 15, 2021 at 01:54:31AM +0200, Vladislav Shpilevoy wrote:
> Good job on the patch!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 5cf067453..ded20315b 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -252,6 +252,34 @@ mem_set_str0_allocated(struct Mem *mem, char *value)
> >  	mem->flags |= MEM_Term;
> >  }
> >  
> > +int
> > +mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
> > +{
> > +	if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
> > +		mem_clear(mem);
> > +	bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
> > +			    mem->z == value;
> > +	if (sqlVdbeMemGrow(mem, len, is_own_value) != 0)
> > +		return -1;
> 
> I would propose this change:
> 
> ====================
> @@ -256,14 +256,14 @@ mem_set_str0_allocated(struct Mem *mem, char *value)
>  int
>  mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
>  {
> -	if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
> -		mem_clear(mem);
> -	bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
> -			    mem->z == value;
> -	if (sqlVdbeMemGrow(mem, len, is_own_value) != 0)
> +	if ((mem->flags & (MEM_Str | MEM_Blob)) != 0 && mem->z == value) {
> +		/* Own value, but might be ephemeral. Make it own if so. */
> +		return sqlVdbeMemGrow(mem, len, 1);
> +	}
> +	mem_clear(mem);
> +	if (sqlVdbeMemGrow(mem, len, 0) != 0)
>  		return -1;
> -	if (!is_own_value)
> -		memcpy(mem->z, value, len);
> +	memcpy(mem->z, value, len);
>  	mem->n = len;
>  	mem->flags = MEM_Str;
>  	mem->field_type = FIELD_TYPE_STRING;
> ====================
> 
> The same for the bins.
> 
> > +	if (!is_own_value)
> > +		memcpy(mem->z, value, len);
> > +	mem->n = len;
> > +	mem->flags = MEM_Str;
> > +	mem->field_type = FIELD_TYPE_STRING;
> > +	return 0;
> > +}