Re: [Tarantool-patches] [PATCH v5 24/52] sql: introduce mem_set_int()

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

Thank you for the review! My answers below.

On Tue, Apr 13, 2021 at 01:32:47AM +0200, Vladislav Shpilevoy wrote:
> Good job on the fixes!
> 
> See 3 comments below.
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 937fa3272..075000218 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -1384,8 +1393,9 @@ vdbe_mem_numerify(struct Mem *mem)
> >  	if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) != 0)
> >  		return 0;
> >  	if ((mem->flags & MEM_Bool) != 0) {
> > -		mem->u.u = mem->u.b;
> > -		MemSetTypeFlag(mem, MEM_UInt);
> > +		mem->u.u = (uint64_t)mem->u.b;
> > +		mem->flags = MEM_UInt;
> > +		mem->field_type = FIELD_TYPE_UNSIGNED;
> 
> 1. Field type wasn't set before. Why did you change that?
> 
Since now I cannot say where field-type and MEM-type are compatible and where
they are not, I just set field-type everywhere. Also, since there is no proper
rule to determine field-type of the MEM I set the smallest field-type possible
for the value of the MEM. This looks like something useless for me, but it is
better than checking if field-type is field_type_MAX everywhere end set
field-type in case it isn't.

> >  		return 0;
> >  	}
> >  	assert((mem->flags & (MEM_Blob | MEM_Str)) != 0);
> > @@ -1454,20 +1464,23 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> >  			return 0;
> >  		}
> >  		if ((pMem->flags & MEM_Bool) != 0) {
> > -			pMem->u.u = pMem->u.b;
> > -			MemSetTypeFlag(pMem, MEM_UInt);
> > +			pMem->u.u = (uint64_t)pMem->u.b;
> > +			pMem->flags = MEM_UInt;
> > +			pMem->field_type = FIELD_TYPE_UNSIGNED;
> 
> 2. Ditto.
> 
Same.

> >  			return 0;
> >  		}
> > @@ -1801,10 +1826,15 @@ mem_convert_to_integer(struct Mem *mem)
> >  	double d = mem->u.r;
> >  	if (d >= (double)UINT64_MAX || d < (double)INT64_MIN)
> >  		return -1;
> > -	if (d < (double)INT64_MAX)
> > -		mem_set_int(mem, (int64_t) d, d < 0);
> > -	else
> > -		mem_set_int(mem, (uint64_t) d, false);
> > +	if (d < 0.) {
> > +		mem->u.i = (int64_t)d;
> > +		mem->flags = MEM_Int;
> > +		mem->field_type = FIELD_TYPE_INTEGER;
> > +	} else {
> > +		mem->u.u = (uint64_t)d;
> > +		mem->flags = MEM_UInt;
> > +		mem->field_type = FIELD_TYPE_UNSIGNED;
> 
> 3. Previously it was FIELD_TYPE_INTEGER in both cases. Why
> did you change that?
Same.