Re: [Tarantool-patches] [PATCH v5 12/52] sql: introduce mem_is_*() functions()

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

Thanks for the fixes!

>> For the integers we have several functions because we split
>> unsigned, signed, and always negative integers. So we would
>> need more int-like names. For instance,
>>
>> 	mem_set_uint(uint64_t) - for MEM_UInt.
>> 	mem_set_nint(int64_t) - for MEM_Int.
>> 	mem_set_int(int64_t) - for both, checks the sign inside.
>> 	mem_set_sint(int64_t, bool) - for both, takes the sign flag
>> 	                              in the second argument
>>
>> This can be discussed. The main point - shorter is better IMO. 
>>
> I do not hink that splitting is needed. I see it more like field_type -> name of
> function + some functions for internal use.

This does not work already, because MEM_Int != FIELD_TYPE_INTEGER and
mem_is_int() does not check for MEM_Int only.

>>>  	}
>>>  }
>>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>>> index ec6aaab64..abc9291ef 100644
>>> --- a/src/box/sql/mem.c
>>> +++ b/src/box/sql/mem.c
>>> @@ -37,6 +37,142 @@
>>>  #include "box/tuple.h"
>>>  #include "mpstream/mpstream.h"
>>>  
>>> +bool
>>> +mem_is_null(const struct Mem *mem)
>>> +{
>>> +	return (mem->flags & MEM_Null) != 0;
>>> +}
>>
>> 4. Maybe better move them all to mem.h. These one-liners easily
>> can be inlined (the ones which are <= 3 lines long could be moved).
>>
> In one of the patches I move MEM types to mem.c so they are not visible from
> outside anymore. I think it is right way, at least for now. We may return
> MEM types back after we convert them to enum, so there won't be a possiblity
> of setting two or more MEM types at the same moment.

But there is now already. AFAIS, MEM_Str might be set along with some
other type. Or was it fixed somewhere in this patchset? See one of
my comments below.

>>> +}
>>> +
>>> +bool
>>> +mem_is_frame(const struct Mem *mem)
>>> +{
>>> +	return (mem->flags & MEM_Frame) != 0;
>>> +}
>>> +
>>> +bool
>>> +mem_is_undefined(const struct Mem *mem)
>>> +{
>>> +	return (mem->flags & MEM_Undefined) != 0;
>>> +}
>>> +
>>> +bool
>>> +mem_is_static(const struct Mem *mem)
>>> +{
>>> +	return (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
>>> +	       (mem->flags & MEM_Static) != 0;
>>> +}
>>> +
>>> +bool
>>> +mem_is_ephemeral(const struct Mem *mem)
>>> +{
>>> +	return (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
>>> +	       (mem->flags & MEM_Ephem) != 0;
>>
>> 7. How can it be that MEM_Ephem is set, but Str/Blob are not?
>>
> There is actually a possiblity. After sqlVdbeMemAboutToChange() is called MEM
> may become invalid after which MEM_Undefined is changed to MEM_Ephem and then

Where is undefined changed to ephem?

> MEM become valid again. For now I disable this SCopyFrom mechanism, but did
> not remove it completely. May be we will enable it later.

<...>

See 4 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 805dc7054..25b2e75ee 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -40,6 +40,149 @@

<...>

> +
> +bool
> +mem_is_map(const struct Mem *mem)
> +{
> +	return (mem->flags & MEM_Blob) != 0 &&
> +	       (mem->flags & MEM_Subtype) != 0 &&

1. You could check that in one operation:

	(mem->flags & (MEM_Blob | MEM_Subtype)) == (MEM_Blob | MEM_Subtype)

> +	       mem->subtype == SQL_SUBTYPE_MSGPACK &&
> +	       mp_typeof(*mem->z) == MP_MAP;
> +}

<...>

> +
> +bool
> +mem_is_cleared(const struct Mem *mem)
> +{
> +	return (mem->flags & MEM_Null) != 0 && (mem->flags & MEM_Cleared) != 0;

2. Can be 1 operation:

	(mem->flags & (MEM_Null | MEM_Cleared)) == (MEM_Null | MEM_Cleared)

But another question is how is it possible that Cleared is set, but
Null isn't?

> +}
> +
> +bool
> +mem_is_zerobin(const struct Mem *mem)
> +{
> +	return (mem->flags & MEM_Blob) != 0 && (mem->flags & MEM_Zero) != 0;

3. The same, can be done in one operation. And the same question - how is it
possible that Zero is set, but Blob isn't?

> +}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 7cc72dc38..f054a0f43 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c> @@ -1884,21 +1868,13 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  				goto compare_op;
>  			}
>  		} else if (type == FIELD_TYPE_STRING) {
> -			if ((flags1 & MEM_Str) == 0 &&
> -			    (flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> -				testcase( pIn1->flags & MEM_Int);
> -				testcase( pIn1->flags & MEM_Real);
> +			if (!mem_is_str(pIn1) && mem_is_num(pIn1)) {

4. Are going to do anything with that hack when a string can be stored in
the same mem as the original value? Otherwise you can see yourself how
ugly and confusing the 'mem_is' checks might look.

Besides, all the mem functions doing something with the mem based on its
pure types mask won't work on such mutant mems I suppose. Because there
is more than 1 type.