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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

> 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.

'Neither operand is NULL' - this is 'else' branch of
"(flags1 | flags3)&MEM_Null", so I guess it is obvious.

> -                * 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.

They can not be == FIELD_TYPE_MASK. Please, again, do not try to
write here any constants, including bit counts. A definition named
MASK is already enough to make it clear, that p5 'and' mask give a
type.

>                   */
> -               enum field_type type = pOp->p5 & 15;
> +               enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
> 

Please, apply the diff below. I made enum field_type_mask an
anonymous enum, as we usually do with independent constants. Also, I
used FIELD_TYPE_MASK instead of 15 in static assertion, and removed
the comment from vdbe.c about what a mask is.

diff --git a/src/box/field_def.h b/src/box/field_def.h
index 3b8eb0ccf..3eda0e38f 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -87,21 +87,22 @@ affinity_type_str(enum affinity_type type);
  
  /** \endcond public */
  
+enum {
+	/**
+	 * This mask allows to store in VdbeOp.p5 operand of
+	 * OP_Eq, OP_Lt etc opcodes field type alonside with
+	 * flags.
+	 */
+	FIELD_TYPE_MASK = 15
+};
+
  /**
   * 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 "\
+static_assert(field_type_MAX <= FIELD_TYPE_MASK, "values of enum field_type "\
  	      "should fit into 4 bits of VdbeOp.p5");
  
-/**
- * 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 dae05e80f..a66ab4992 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2151,14 +2151,6 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
  			break;
  		}
  	} else {
-		/*
-		 * Neither operand is NULL. Do a comparison.
-		 * 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 & FIELD_TYPE_MASK;
  		if (sql_type_is_numeric(type)) {
  			if ((flags1 | flags3)&MEM_Str) {