[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code

[n.pettik]

>>> I propose you to either come up with a better solution or to use one of
>>> my solutions:
>>> 
>>> * return AFFINITY_MASK as P5_FIELD_TYPE_MASK, and add a static assertion
>>>  that P5_FIELD_TYPE_MASK < (1 << bit_count(field_type)).
>>> 
>>>  Or write in field_def.h something like this:
>>> 
>>>      static_assert(bit_count(field_type) <= 4,
>>>                    "size of enum field_type is used in "\
>>>                    "VdbeOp.p5 to fit it into 4 bits”);
>> To be honest I don’t understand these bit assertions.
>> Why can’t we simply check that field_type_MAX <= 15?
>> diff --git a/src/box/field_def.c b/src/box/field_def.c
>> index b34d2ccd9..1a8074650 100644
>> --- a/src/box/field_def.c
>> +++ b/src/box/field_def.c
>> @@ -33,6 +33,13 @@
>>  #include "trivia/util.h"
>>  #include "key_def.h"
>>  +/**
>> + * For detailed explanation see context of OP_Eq, OP_Lt etc
>> + * opcodes in vdbe.c.
>> + */
>> +static_assert(field_type_MAX <= 15, "values of enum field_type "\
>> +             "should fit into 4 bits of VdbeOp.p5");
>> +
>> Also, as you may notice, I placed it to field_def.c
>> Seems that field_def.h is used to generate module.h,
>> which in turn doesn’t tolerate this kind of assert.
> 
> field_def.h is not copied to module.h entirely. Only
> the code between /** \cond public */ and /** \endcond public */.
> Please, put this assertion below /** \endcond public */.

Didn’t pay attention to that. Fixed:

diff --git a/src/box/field_def.c b/src/box/field_def.c
index 1a8074650..b34d2ccd9 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -33,13 +33,6 @@
 #include "trivia/util.h"
 #include "key_def.h"
 
-/**
- * For detailed explanation see context of OP_Eq, OP_Lt etc
- * opcodes in vdbe.c.
- */
-static_assert(field_type_MAX <= 15, "values of enum field_type "\
-             "should fit into 4 bits of VdbeOp.p5");
-
 const char *mp_type_strs[] = {
        /* .MP_NIL    = */ "nil",
        /* .MP_UINT   = */ "unsigned",
diff --git a/src/box/field_def.h b/src/box/field_def.h
index 93e38ea55..358e35f69 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -87,6 +87,13 @@ affinity_type_str(enum affinity_type type);
 
 /** \endcond public */
 
+/**
+ * For detailed explanation see context of OP_Eq, OP_Lt etc
+ * opcodes in vdbe.c.
+ */
+static_assert(field_type_MAX <= 15, "values of enum field_type "\
+             "should fit into 4 bits of VdbeOp.p5");
+
 extern const char *field_type_strs[];
 
 extern const char *on_conflict_action_strs[];

>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 80d2fd0aa..358b69754 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2164,17 +2151,24 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>> 			break;
>> 		}
>> 	} else {
>> -		/* Neither operand is NULL.  Do a comparison. */
>> -		affinity = pOp->p5 & AFFINITY_MASK;
>> -		if (affinity>=AFFINITY_INTEGER) {
>> +		/*
>> +		 * Neither operand is NULL. Do a comparison.
>> +		 * 15 is 1111 in a binary form.
>> +		 * Since all existing types can be fitted in 4 bits
>> +		 * (field_type_MAX == 10), it is enough to 'and'
>> +		 * p5 with this constant. Node that all other flags
>> +		 * that can be stored in p5 are >= 16.
> 
> Firstly, as I said in the previous review, field_type_MAX == 9.
> Secondly, please, do not use a constant. All of them (15, 111, 10, 16).
> Instead of this huge comment full of constants just create a enum
> value with an appropriate name, like FIELD_TYPE_MASK = 15, it is
> even ok to put this name into field_def.h. Thirdly, not all p5 flags
> are >= 16. But for these concrete opcodes it is so, luckily. At
> least we have OPFLAG_NCHANGE, OPFLAG_EPHEM, OPFLAG_SEEKEQ etc -
> they all are <= 15.

Is this variant OK?

+/**
+ * This mask allows to store in p5 operand of OP_Eq, OP_Lt etc
+ * opcodes field type alonside with other flags.
+ */
+enum field_type_mask {
+       FIELD_TYPE_MASK = 15
+};
+
 extern const char *field_type_strs[];
 
 extern const char *on_conflict_action_strs[];
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 358b69754..dae05e80f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2153,13 +2153,13 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
        } else {
                /*
                 * Neither operand is NULL. Do a comparison.
-                * 15 is 1111 in a binary form.
-                * Since all existing types can be fitted in 4 bits
-                * (field_type_MAX == 10), it is enough to 'and'
-                * p5 with this constant. Node that all other flags
-                * that can be stored in p5 are >= 16.
+                * Since all existing types can be fitted in 4
+                * bits (field_type_MAX == 9), it is enough to
+                * 'and' p5 with field mask. Note that values of
+                * other flags that can be stored in p5 of these
+                * opcodes are >= FIELD_TYPE_MASK.
                 */
-               enum field_type type = pOp->p5 & 15;
+               enum field_type type = pOp->p5 & FIELD_TYPE_MASK;